Not adding blob sidecar for EIP-4844 transaction when encoding for signing

[mskrzypkows]

What does this PR do?

Fixes encoding of EIP-4844 transaction for signing

Where should the reviewer start?

Transaction4844::asRlpValues

Why is it needed?

I've created a PR to the web3signer repository: https://github.com/Consensys/web3signer/pull/1096/files#diff-e3e39c1cb1261aafa22d5bc9fb0e0e28d222a209801620a9242e77c37dfe259eR64
And the serialisation produced wrong signer. When I digged into web3j I realised that for signing EIP-4844 an additional list is added: List<RlpType> result = new ArrayList<>(); this list shifts signature bytes which are wrongly interpreted when deserialising. That's why I don't add this sidecar list when creating signature. Let me know if it's correct approach.

Checklist

• I've read the contribution guidelines.
• I've added tests (if applicable).
• I've added a changelog entry if necessary.

issue: #2195

[siladu]

For reference, here's some relevant Besu classes:
https://github.com/hyperledger/besu/blob/main/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/EncodingContext.java

BLOCK_BODY context uses https://github.com/hyperledger/besu/blob/main/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobTransactionEncoder.java

POOLED_TRANSACTION context uses
https://github.com/hyperledger/besu/blob/main/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/encoding/BlobPooledTransactionEncoder.java

Besu doesn't have to worry about unsigned transactions though.

[siladu]

There are two RLP representations of a 4844 transaction: one for block body (without sidecar) and one "pooled transaction" for txpool/networking (with sidecar): https://eips.ethereum.org/EIPS/eip-4844#networking

I don't have enough context to know if switching on the presence of a signature is appropriate for that difference. Is it possible a client may want to pass in a signature for a pooled transaction?
Perhaps this class needs to expose the two possible RLP representations to the clients since maybe only the clients will have the appropriate context.

[mskrzypkows]

Good point — I'm not sure about the signature of the pooled transaction. What I have verified is the signing of the block body, using this signature for the pooled transaction when passing it to Geth. With this signature, the signer of the pooled transaction was decoded correctly and the transaction was accepted. However, when the whole pooled transaction was signed, the signer was decoded incorrectly. It looked like sending from different address than I did.

[siladu]

when the whole pooled transaction was signed, the signer was decoded incorrectly.

That's a little surprising to me given what I was reading yesterday, I would have guessed your context was block body. But I'm not very close to this code so just wanted to highlight the two possible contexts.

[mskrzypkows]

I know this change requires some tests modifications, but first I want your confirmation for what has been changed, and then I'll modify tests.

[gtebrean]

@mskrzypkows thanks for PR and investigation, I also took a bit of time to remember and understand the topic.
Going through the comments I see your points, but I'm wondering how was this hidden for so long, we tested it against Besu. Here is also a blogpost https://blog.web3labs.com/eip-4844-blob-transactions-support-in-web3j/.

I'm thinking the maybe this was the cause that we missed during the implementation: https://eips.ethereum.org/EIPS/eip-4844#networking. Here it shows a rlp() over those 3 but in fact if we read the EIP carefully the side car shouldn't be actually be rlp encoding because it shouldn't be broadcasted...

I see in your web4signer that you call encoding with signatureData, and here you moved that encode in the if block. In your tests were you able to encode and also decode and get the data after the transaction was submitted?
Did your tests cover the case when signatureData wasn't null?

[mskrzypkows]

I have verified it with and without signatureData in web3signer:

   byte[] bytesToSign = transaction.rlpEncode(null);   // without signature
    bytesToSign = prependEip4844TransactionType(bytesToSign);

    final SignatureData signatureData = sign(transaction.sender(), bytesToSign);

    byte[] serializedBytes = transaction.rlpEncode(signatureData);   // with signature
    return prependEip4844TransactionType(serializedBytes);

So first I pass null, encode without a blob sidecar then I encode whole tx with a sidecar. I've verifed the singature and was fine and other tx fileds too, what I haven't checked are blobs binary correctness after getting back signed transaction. I can check it.

[siladu]

So first I pass null, encode without a blob sidecar then I encode whole tx with a sidecar. I've verifed the singature and was fine and other tx fileds too, what I haven't checked are blobs binary correctness after getting back signed transaction. I can check it.

@mskrzypkows Thanks, I understand the flow now and agree the changes with the signature null check make sense for this uses case.

if we read the EIP carefully the side car shouldn't be actually be rlp encoding because it shouldn't be broadcasted

@gtebrean can you elaborate on this? I can see that Besu indeed broadcasts the encoded side car with the PooledTransaction message gossip.

Is this Transaction4844 type only intended to be used for signing and sending transactions? If so then I think the PR makes sense as is.
The block_body-encoded transaction without the side car but with the signature could be regarded as an internal client concern.
However, if you wanted to use this code to get the RLP for a block_body-encoded transaction then you might need to expose a different variant of asRlpValues.

[gtebrean]

Hey, sorry, I was off for a bit.

@siladu this is what made me think that sidecar shouldn't be actually encoded:
"On the consensus layer the blobs are referenced, but not fully encoded, in the beacon block body. Instead of embedding the full contents in the body, the blobs are propagated separately, as “sidecars”." [https://eips.ethereum.org/EIPS/eip-4844#consensus-layer-validation]

@mskrzypkows if the tests done worked fine on your side and as this is the first feedback that we get for the EIP 4844 in Web3j, let's give it a try! Please go ahead with this approach, write/update the unit tests and I will merge it asap to include it in this week release.

[gtebrean]

@mskrzypkows btw in the meantime I see this PR was raised, #2205, could you please let me know if this impacts you in any way?

[mskrzypkows]

@mskrzypkows btw in the meantime I see this PR was raised, #2205, could you please let me know if this impacts you in any way?

@gtebrean I've commented that PR, I think that there is a typo.

@mskrzypkows if the tests done worked fine on your side and as this is the first feedback that we get for the EIP 4844 in Web3j, let's give it a try! Please go ahead with this approach, write/update the unit tests and I will merge it asap to include it in this week release.

OK, sounds good. I'll try to add tests this week.

[siladu]

"On the consensus layer the blobs are referenced, but not fully encoded, in the beacon block body. Instead of embedding the full contents in the body, the blobs are propagated separately, as “sidecars”." [https://eips.ethereum.org/EIPS/eip-4844#consensus-layer-validation]

@gtebrean
Ah ok that is talking about the consensus layer (beacon) chain rather than the execution layer block bodies.
I have only been thinking in terms of execution layer (which is where transactions are sent).
I would expect no interactions with Transaction objects on the consensus layer AFAIK, they are simply opaque bytes contained within an execution layer block body aka "payload" IIUC.

[mskrzypkows]

@conor10, @NickSneo Could you please review it?

[alexghr]

Hey folks, at Aztec we're adding support for remote signers to our node for signing transactions and this is currently blocking the PR for EIP-4844 txs in Web3Signer.

Any chance we could get this PR merged in and a new version of the library released? Thanks! 😁

[just-mitch]

Hey @gtebrean , can you give any clarity here? Are we awaiting reviews? Is there a better way for the contributors to talk through changes? I saw there is a recurring community call for this repo, but it doesn't meet again until mid October.

We'd prefer not to fork, so if there is a way to get this into the main release, that would be good.

[gtebrean]

Hi all, as we mentioned on discord since 2 months we have issue with the release process and because of that we were blocked, the PR is fine.
Now we managed to fix the SNAPSHOT release part and for now you can use it in your project with this version 4.14.1-SNAPSHOT, don't forget to add the repo spec:

repositories>

Central Portal Snapshots
central-portal-snapshots
https://central.sonatype.com/repository/maven-snapshots/

false


true


</repositories

hopefully will have the release also asap

[mskrzypkows]

Thanks! What's your Discord server?