Dont append full wrapped blobtx to filters #9471
Conversation
|
Hi @somnathb1 just wondering if this will also fix the issue posted here: |
|
Hey @somnathb1, I've just stumbled upon the following issue #9475, I think it's cause also might be related to the usage of blob RLP wrappers in txpool. Can you kindly check if this PR will fix it as well? |
| @@ -528,7 +528,7 @@ func (ff *Filters) OnNewTx(reply *txpool.OnAddReply) { | |||
| if len(rlpTx) == 0 { | |||
| continue | |||
| } | |||
| txs[i], decodeErr = types.DecodeWrappedTransaction(rlpTx) | |||
| txs[i], decodeErr = types.DecodeTransaction(rlpTx) | |||
There was a problem hiding this comment.
This change looks problematic to me. The unfortunate thing is that we use "wrapped" in two ways:
A) Whether a typed (such as EIP-1559 or blob) transaction, whose serialization starts with its type byte, is additionally wrapped into the RLP string.
B) Whether a blob transactions is "wrapped" to contain blobs/commitments/proofs.
While I agree that with this PR we need "unwrapped" in sense B here, we still need DecodeWrappedTransaction in sense A – otherwise EIP-1559 (type-2) transactions will fail I suspect.
I suggest adding a new parameter to UnmarshalWrappedTransactionFromBinary (and hence DecodeWrappedTransaction) that explicitly controls whether we expect type-3 (blob) transactions with (deserialized into BlobTxWrapper) or without (deserialized into BlobTx) blobs. Currently UnmarshalWrappedTransactionFromBinary implicitly does the former.
There was a problem hiding this comment.
Qq: Wrapped in 2 ways? There is wrapped blobTx, and nothing else that is "wrapped" additionally
Your suggestion for the case B is wrapping of tx_payload and blobs, commitment, proofs into one RLP is what UnmarshalWrappedTransactionFromBinary deals with. The rest of the logic is the same.
If for any reason type-2 transactions are more wrapped, then the existing logic wouldn't have worked either.
Also here, OnNewTx will only process elements from txpool's reply. I am ensuring that the wrapped version of blobTx isn't fed into that.
There was a problem hiding this comment.
OK, I was confused by the comment of DecodeWrappedTransaction. I thought that DecodeWrappedTransaction differed from DecodeTransaction in how they treat RLP envelops of typed transactions. Take a look at TestEIP2718TransactionEncode, for example. What is called "binary" representation is its serialization prescribed by EIP-2718. It's used in some places, for example when calculating the transaction root of a block header. However, in other places (e.g. when sent over network in eth/68) this "binary" representation has to be additionally "wrapped" with an RLP prefix to make it a valid RLP. You can see in TestEIP2718TransactionEncode that the "RLP representation" is the same as the "binary representation" prefixed by b866. Note that the above is only a concern for typed transactions, not for legacy ones.
Anyway, it looks like in reality DecodeWrappedTransaction behaves the same as DecodeTransaction re. the RLP envelope.
While reviewing PR #9471, I realized that `DecodeWrappedTransaction` was inconsistent. In one case it'd call `UnmarshalWrappedTransactionFromBinary`, which deserialized type-3 transactions into `BlobTxWrapper`, and in another case it'd call `DecodeRLPTransaction`, which deserialized type-3 transactions into `BlobTx`. This PR makes sure that `DecodeWrappedTransaction` always deserializes into `BlobTxWrapper`.
In txpool, the rlp is appended to be broadcasted as is. This PR removes the unnecessary parts of the wrapped rlp, namely everything but the payload part.
There is a redundancy in rlp libraries inside and outside
erigon-libdirectory. I have added some redundant code here, for future reference withincore/types