mattsse
commented
2 years ago probably something's off with the conversion to typedtransaction and/or its hash
Open meetmangukiya opened 2 years ago
mattsse
commented
2 years ago probably something's off with the conversion to typedtransaction and/or its hash
gakonst
commented
2 years ago cc @Rjected maybe related to the rlp ser/de
Rjected
commented
2 years ago hmm, it would be helpful to have a tx example to test conversion / hashing if you have one
meetmangukiya
commented
2 years ago use ethers::types::H256;
use ethers::providers::Provider;
use std::sync::Arc;
use ethers::providers::Middleware;
#[tokio::main]
async fn main() -> eyre::Result<()> {
let provider = Arc::new(Provider::try_from("https://mainnet.infura.io/v3/c60b0bb42f8a4c6481ecd229eddaca27")?);
let tx = provider.get_transaction("0x655357c04593c85e84cadd945bde83386d6519b6cbc90380a120ecb2f5930253".parse::<H256>()?).await?.unwrap();
let from = tx.recover_from()?;
println!("recovered: {:?}", from);
println!("original: {:?}", tx.from);
assert_eq!(from, tx.from);
Ok(())
}Ok I figured it out, it is once again the chain id not being serialized properly before computing the sighash. The rlp encoding methods currently encode the chain_id in the following way before hashing:
/// Gets the transaction's RLP encoding, prepared with the chain_id and extra fields for
/// signing. Assumes the chainid exists.
pub fn rlp(&self) -> Bytes {
let mut rlp = RlpStream::new();
rlp.begin_list(NUM_TX_FIELDS);
self.rlp_base(&mut rlp);
// Only hash the 3 extra fields when preparing the
// data to sign if chain_id is present
rlp_opt(&mut rlp, &self.chain_id);
rlp.append(&0u8);
rlp.append(&0u8);
rlp.out().freeze().into()
}EIP155 means transactions need a populated chain id parameter for any signing or recovery related operations to work.
As an aside, maybe chain_id should not be an Option because of this?
The problem is that rlp_opt will serialize the chain id as an empty string (0x80) if it is None. This one byte difference causes an incorrect sighash and recovery failure.
Populating the chain_id in the example causes the code example to succeed:
use ethers::types::H256;
use ethers::providers::Provider;
use std::sync::Arc;
use ethers::providers::Middleware;
#[tokio::main]
async fn main() -> eyre::Result<()> {
let provider = Arc::new(Provider::try_from("https://mainnet.infura.io/v3/c60b0bb42f8a4c6481ecd229eddaca27")?);
let tx = provider.get_transaction("0x655357c04593c85e84cadd945bde83386d6519b6cbc90380a120ecb2f5930253".parse::<H256>()?).await?.unwrap();
tx.chain_id = Some(provider.get_chainid().await?);
let from = tx.recover_from()?;
println!("recovered: {:?}", from);
println!("original: {:?}", tx.from);
assert_eq!(from, tx.from);
Ok(())
}recovered: 0xc4b732fd121f2f3783a9ac2a6c62fd535fd13fda
original: 0xc4b732fd121f2f3783a9ac2a6c62fd535fd13fdaSo maybe the solution here is to change get_transaction to populate the chain id, since the provider is responsible for filling the transaction and there is no way to populate it later.
mattsse
commented
2 years ago wait, why's there even a chain_id field in the Transaction response object?
mmm, this doesn't look very sound to me to recover from the response directly because of this
what ethers has is
TypedTransaction**Request** typeTransaction Response type as returned from the eth APIbut what's missing is something like
TypedTransaction that represents the Transaction Response type, we use this in anvil
which anvil uses to recover the sender, this is what eth_sendRawTransaction decodes to
so perhaps we can do
chain_id fieldTypedTransaction from anvil to ethers (ref https://github.com/foundry-rs/foundry/issues/2017)Transaction::into_typed(chain_id) -> TypedTransactionRjected
commented
2 years ago I could be wrong, but I'm not sure that any provider actually sends a chainId in a Transaction response, so I also feel like some of the types should be revised here.
We should definitely introduce a TypedTransaction, or generally a typed signed transaction type like in anvil into ethers.
Encoding and decoding methods on Request types could also be moved to their appropriate place:
rlp_signed is now an Encodable implementation on TypedTransaction.decode_signed_rlp is now a Decodable implementation on TypedTransaction.
Version
Platform
Description
Transaction::recover_fromdoesn't work well for recovering from for legacy txs. Have to look into it. Opening to track the bug.