-
Notifications
You must be signed in to change notification settings - Fork 284
[WIP] caps/eth.md: cell exchange #263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
caps/eth.md
Outdated
The `cells` element is a bitmap marking the indices of cells stored by the sending peer. | ||
For each cell stored by the peer the corresponding bit is set. Note that blob transactions | ||
with same cells field is being batched together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can the schema proposed there deal with (a) max 6 blobs per tx and (b) multiple type 3 txs being announced in the same message? Or does "Note that blob transactions with same cells field is being batched together." imply that if cell availability differs across txs, we'd announce them separately?
- I think it's safe to assume that nodes will want to sample the same cell indices for all blobs in a single type 3 tx, which makes the
cells
naming a bit confusing IMO, but also not a dealbreaker (technically they're columns, but also technically columns don't exist until the blobs are packed in a block).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we can assume that nodes will try to fetch the same indices for all transactions in most cases. That is why I designed this in a batched form. So, regarding the first question, yes, we would announce them separately, but we can easily assume that most transactions would be batched together and there would not be much fragmentation. I also agree on the naming. cell-indices
, custody-indices
, and mask
could be candidates, but we could also just leave it as it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. We might want nodes to sample a few random cells from peers holding the full payload in addition to their custody set. This mechanism is a sentinel against selective data serving attacks. Otherwise an attacker could repeatedly advertise full availability while holding only the exact subset of cells they know we'll request every time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a type 3 tx can have several blobs, we should clarify whether the bitmap is at the tx or at the blob granularity.
For our current purposes, tx granularity looks fine, and then the meaning of a bit is that that the node has the corresponding cell of all blobs in the tx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since a type 3 tx can have several blobs, we should clarify whether the bitmap is at the tx or at the blob granularity. For our current purposes, tx granularity looks fine, and then the meaning of a bit is that that the node has the corresponding cell of all blobs in the tx.
Thank you, I added a more detailed explanation about this in the introduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. We might want nodes to sample a few random cells from peers holding the full payload in addition to their custody set. This mechanism is a sentinel against selective data serving attacks. Otherwise an attacker could repeatedly advertise full availability while holding only the exact subset of cells they know we'll request every time.
I’m not sure how much random sampling would be needed given that we have the full payload node count check too, but I think it can be supported under this design. A node can select random columns and perform sampling over the transaction batch, and if an attacker skips the cell indices of the transaction they want to hide, the node would request that column from another peer and ultimately be able to drop the malicious transaction.
caps/eth.md
Outdated
|
||
'eth' is a protocol on the [RLPx] transport that facilitates exchange of Ethereum | ||
blockchain information between peers. The current protocol version is **eth/69**. See end | ||
blockchain information between peers. The current protocol version is **eth/70**. See end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already eth/70 proposals in progress, so the number will be different
We can update at the end in the doc, but maybe remove from title to avoid confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all eth/70 numbering in this PR and on doc so that we can add it back at the end.
a0439c4
to
6caf1bc
Compare
caps/eth.md
Outdated
relayed by the peer. | ||
|
||
For additional rules for blob transactions, refer to the [blob transaction and cell exchange] | ||
section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add a link to the section that starts right below.
caps/eth.md
Outdated
A node must specify no more than 4 (TBD) indices per request. To manage uplink bandwidth usage, | ||
a node may disconnect peers that send excessive requests. This can be enforced by monitoring | ||
metrics such as the number of requested cells over a given period. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to specify such limits where the message is defined.
### NewPooledTransactionHashes (0x08) | ||
|
||
`[txtypes: B, [txsize₁: P, txsize₂: P, ...], [txhash₁: B_32, txhash₂: B_32, ...]]` | ||
`[txtypes: B, [txsize₁: P, txsize₂: P, ...], [txhash₁: B_32, txhash₂: B_32, ...], cells: B_16]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should decide whether there is one cells
bitmap for the whole message, or one cells
bitmap per transaction - in the pool. In the first case, the bitmap should refer to all transactions (or all type 3 transactions) in the pool.
- in the second case, we could have a separate bitmap for each tx. Eventually we could compress a bit by skipping bitmaps for non type3 transactions.
caps/eth.md
Outdated
|
||
The `cells` element is a bitmap marking which cell indices can be fetched from the sending | ||
peer. For each bit set to one, the peer stores the cell at that index in all blobs of the | ||
transaction. A bit must be set only if the peer has the cell at that index in all blobs of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you say "in all blobs of THE TRANSACTION". But the message can refer to more transactions, so the bits should also refer to all transactions (which means all blobs of all included transactions), or we need multiple bitmaps (in that case each bitmap would refer to all blobs or the respective transaction).
peer. For each bit set to one, the peer stores the cell at that index in all blobs of the | ||
transaction. A bit must be set only if the peer has the cell at that index in all blobs of | ||
the transaction. This field is only relevant for those entries that refer to blob | ||
transactions. Blob transactions with the same `cells` field may be announced together in a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, the the intended notion is that the bitmap refers to all blobs of all transactions. Otherwise one would need separate NewPooledTransactionHashes messages for each transaction.
caps/eth.md
Outdated
This message requests the peer to return cell data of the given transaction hashes. | ||
The cells element is a bitmap specifying which cell indices are requested. For each bit | ||
set, the requester asks for the cell at that index from all blobs of the corresponding | ||
transaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, if we speak of "the corresponding transaction", we need [cells_1 :B_16, cells_2: B_16, ...]
Otherwise we should rephrase this so that the bitmap refers to all blobs of all included transactions.
caps/eth.md
Outdated
|
||
A node should either set 4 bits (with probability $1–p$) or 64 bits (with probability $p$) | ||
in the cells field. This mechanism prevents a greedy peer from abusing bandwidth and | ||
encourages collective fetch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to have these numbers directly in the message format specification.
These numbers will change when we change the number of columns. Second, what is a valid use (number of bits) and how it should be used (probability) depends on how we define the protocol logic. To me that would be better in another section about protocol dynamics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first concern can be solved by parameterization which you previously suggested.
For the second concern, since this section describes how each message should be processed and used, it does not seem possible to avoid mentioning it here. So my idea is to briefly state only the limit here first and later provide more protocol related background like why such a limit was chosen (including any future experiments or analyses that may be needed) either in the introduction or in a separate section. What do you think about this approach?
caps/eth.md
Outdated
transactions, in addition to the ordinary transaction exchange. | ||
|
||
Blob transactions contain one or more 128 KiB fixed-size objects called blobs. Blobs can be | ||
split into cells using the erasure code defined in [EIP-7594]. The size of a cell is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while here we can have example numbers, I think later it would be better to parametrize these similar to EIP-7594.
number of cells, related bitmap size, and cell size should all be parametrized I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a parameter table regarding this. I am not entirely sure this is what you meant, but I think its pretty nice since it shows which configurations we need to keep an eye on
caps/eth.md
Outdated
Upon receiving the [NewPooledTransactionHashes] message with new blob transaction hashes, | ||
the node begins fetching cells in parallel with transaction fetching. The node first makes | ||
a probabilistic decision. If it decides to fetch full blobs with probability $p$, it | ||
requests them using the [GetCells] message, setting more than 50% of the total cell indices |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "more than 50%"? Do we even want to allow setting more than 50%? That would be more adequate for some lossy transport, but we don't (yet) have that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my error. Thanks for pointing it out I’ve fixed it.
caps/eth.md
Outdated
the node begins fetching cells in parallel with transaction fetching. The node first makes | ||
a probabilistic decision. If it decides to fetch full blobs with probability $p$, it | ||
requests them using the [GetCells] message, setting more than 50% of the total cell indices | ||
to 1 in the cells field. The recommended probability $p$ is 0.15. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of recommending 0.15 here, I think it is better to have a parameters section that we can update all in one.
Signed-off-by: Csaba Kiraly <[email protected]>
Signed-off-by: Csaba Kiraly <[email protected]>
Signed-off-by: Csaba Kiraly <[email protected]>
Hi @healthykim , I've added a few comments/questions above. I've also made some changes to the test, you can find them here: |
…ested per transaction
This is a draft PR to support cell level messaging on the EL