8000
Skip to content
This repository was archived by the owner on Oct 19, 2024. It is now read-only.

fix DepositTransaction::rlp() to match op-geth - #2644

Merged
mattsse merged 7 commits into
gakonst:masterfrom
noot:master
Oct 23, 2023
Merged

fix DepositTransaction::rlp() to match op-geth#2644
mattsse merged 7 commits into
gakonst:masterfrom
noot:master

Conversation

@noot
@noot noot commented Oct 19, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Motivation

The RLP encoding for Optimism DepositTransactions did not match the op-geth implementation. Deposit transactions created and encoded by ethers-rs were previously not being accepted by op-geth due to this.

Solution

Fix DepositTransaction::rlp() to match op-geth DepositTx::encode(). The types are now encoded to match op-geth: https://github.com/ethereum-optimism/op-geth/blob/optimism/core/types/deposit_tx.go#L29

I generated the expected Go output as follows (place this in op-geth core/types/deposit_tx_test.go):

package types

import (
	"bytes"
	"encoding/hex"
	"math/big"
	"testing"

	"github.com/ethereum/go-ethereum/common"
)

func TestEncodeDepositTx(t *testing.T) {
	to := common.HexToAddress("0xa0Ee7A142d267C1f36714E4a8F75612F20a79720")
	depositTx := &DepositTx{
		SourceHash:          common.HexToHash("0x7113be8bbb6ff4bb99fae05639cf76cdecf5a1afbc033b9a01d8bb16b00b9a80"),
		From:                to,
		To:                  &to,
		Mint:                big.NewInt(10000000000000000),
		Value:               big.NewInt(10000000000000000),
		Gas:                 21000,
		IsSystemTransaction: false,
		Data:                nil,
                // for the with_data test 
                // Data:                []byte("nootwashere"),
	}
	bytes := &bytes.Buffer{}
	depositTx.encode(bytes)
	t.Log(hex.EncodeToString(bytes.Bytes()))
}

Note: the existing test test_rlp_encode_deposited_tx is probably not exactly correct? Should I remove/update it?

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

mattsse
mattsse reviewed Oct 19, 2023
View reviewed changes
@mattsse mattsse left a comment
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the entire optimism integration is a bit horrible...

hmm, I wonder why these fields are even options.

https://github.com/ethereum-optimism/optimism/blob/65ec61dde94ffa93342728d324fecf474d228e1f/specs/deposits.md#l1-attributes-deposited-transaction

but if all of them are set, then this change doesn't have an effect right, because rlp_opt simply calls append, right?

and it looks like the decode function is current because it expects all the fields

so I guess the real question is, should this not even accept options

@noot
noot commented Oct 19, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@mattsse good question, I don't think source_hash and is_system_tx should be Options since they're not in the Go impl. Not sure about mint (the Go impl has it tagged as rlp:"nil" - let me test that)

but if all of them are set, then this change doesn't have an effect right, because rlp_opt simply calls append, right?

the result i'm getting (before the change) is:
f863a07113be8bbb6ff4bb99fae05639cf76cdecf5a1afbc033b9a01d8bb16b00b9a80d594a0ee7a142d267c1f36714e4a8f75612f20a7972094a0ee7a142d267c1f36714e4a8f75612f20a79720872386f26fc10000c8872386f26fc10000c38252088080

compared to the go result of:
f860a07113be8bbb6ff4bb99fae05639cf76cdecf5a1afbc033b9a01d8bb16b00b9a8094a0ee7a142d267c1f36714e4a8f75612f20a7972094a0ee7a142d267c1f36714e4a8f75612f20a79720872386f26fc10000872386f26fc100008252088080

this seems strange, because you're right, it should just be encoding the value inside the option. i'll investigate more

@noot
noot commented Oct 19, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

update, the problems were

  • self.tx.from (it's an option, but needs to be encoded as not an option)
  • self.tx.value (it's an option, but needs to be encoded as not an option)
  • self.tx.gas (it's an Option<U256> but needs to be encoded as u64)

I left source_hash and is_system_tx as unwrap_or_default because they shouldn't be encoded as None ever imo. (maybe we should change those to not be options)

@mattsse
mattsse commented Oct 19, 2023 •
edited
Loading
Copy link
Copy Markdown
Collaborator

I see, I think another problem is Data, which looks like a non optional []byte which is always encoded?

and iirc an empty []byte is encoded as an rlp header, but maybe that's what rlp.append("") does

@noot
noot commented Oct 19, 2023
Copy link
Copy Markdown
Contributor Author

yep i also changed data to use unwrap_or_default, which matches the go encoding when you set it to nil, so i think that's ok - i'm guessing go rlp-encodes []byte(nil) and []byte{} the same?

@mattsse
mattsse commented Oct 19, 2023
Copy link
Copy Markdown
Collaborator

I left source_hash and is_system_tx as unwrap_or_default because they shouldn't be encoded as None ever imo.

yeah, they are required:

https://github.com/anton-rs/op-reth/blob/clabby/fixes/crates/primitives/src/transaction/optimism.rs#L12

(maybe we should change those to not be options)

agree

mattsse
mattsse approved these changes Oct 19, 2023
View reviewed changes
@mattsse mattsse left a comment
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks correct, since all required fields are now encoded.

I think we should consider making them non optional.

pending @DaniPopes

Comment thread ethers-core/src/types/transaction/optimism.rs
rlp_opt(&mut rlp, &self.source_hash);
rlp.append(&self.tx.from);
rlp.append(&self.source_hash.unwrap_or_default());
rlp.append(&self.tx.from.unwrap_or_default());
@mattsse mattsse Oct 19, 2023
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, we need to unwrap the from field because deposit txs don't have a signature...

Comment thread ethers-core/src/types/transaction/optimism.rs
rlp.append(&self.tx.value.unwrap_or_default());
rlp.append(&self.tx.gas.unwrap_or_default().as_u64());
rlp.append(&self.is_system_tx.unwrap_or_default());
rlp.append(&self.tx.data.as_deref().unwrap_or_default());
@mattsse mattsse Oct 19, 2023
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe rlp_opt was correct here because it falls back to rlp.append("") which is equivalent to rlp.append(&[])

but this is better

Comment thread ethers-core/src/types/transaction/optimism.rs
Comment on lines +55 to +56
rlp.append(&self.tx.value.unwrap_or_default());
rlp.append(&self.tx.gas.unwrap_or_default().as_u64());
@mattsse mattsse Oct 19, 2023
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, rlp_opt would fallback to an empty string and 0 is also encoded as empty string code I believe.

but this is better.

@noot
noot commented Oct 19, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor Author

@mattsse i went ahead and updated source_hash and is_system_tx to not be options, let me know if you want me to leave it or change back

@mattsse
mattsse commented Oct 19, 2023
Copy link
Copy Markdown
Collaborator

I think that's the right decision

@noot
noot commented Oct 19, 2023
Copy link
Copy Markdown
Contributor Author

@mattsse what's a good move for fixing the serde_txpool_content test? it appears to me that the existing json-string in that test should only be using if #[cfg(not(feature = "optimism"))], i can make another json string for if the optimism feature is enabled (or just throw a #[cfg(not(feature = "optimism"))] on the whole test?)

@mattsse
mattsse commented Oct 23, 2023
Copy link
Copy Markdown
Collaborator

@noot sorry, for late reply,

we can remove this test entirely, no longer needed since we're transitioning away from these types for alloy migration anyway

@noot
noot commented Oct 23, 2023
Copy link
Copy Markdown
Contributor Author

@mattsse np, test removed!

@mattsse
mattsse 9927 commented Oct 23, 2023
Copy link
Copy Markdown
Collaborator

fyi @merklefruit

@mattsse
mattsse merged commit 4acfa9d into gakonst:master Oct 23, 2023
@SuperFluffy SuperFluffy mentioned this pull request Nov 8, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@mattsse mattsse mattsse approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@noot @mattsse
0