Skip to content
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

Regression: "Failed to decode transaction" #9046

Closed
2 tasks done
kayabaNerve opened this issue Oct 7, 2024 · 6 comments
Closed
2 tasks done

Regression: "Failed to decode transaction" #9046

kayabaNerve opened this issue Oct 7, 2024 · 6 comments
Labels
T-bug Type: bug

Comments

@kayabaNerve
Copy link
Contributor

Component

Anvil

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

Latest

What command(s) is the bug in?

No response

Operating System

Linux

Describe the bug

Since 4 days ago, the version of anvil downloaded by installing foundryup via the script at https://foundry.paradigm.xyz (and then running it) has yielded

ErrorResp(ErrorPayload { code: -32602, message: "Failed to decode transaction", data: None })

when I run the following block of code:

        let provider = RootProvider::<_, Ethereum>::new(
          ClientBuilder::default().transport(SimpleRequest::new(rpc_url.clone()), true),
        );
        let _ = provider.send_raw_transaction(tx).await.unwrap();

The tx value in that case is an opaque byte array generated as follows:

        let tx = TxLegacy {
          chain_id: None,
          nonce: *nonce,
          gas_price: 1_000_000_000u128,
          gas_limit: 200_000u128,
          to: TxKind::Call(router_addr.into()),
          // 1 ETH
          value: one_eth,
          input: ethereum_serai::router::abi::inInstructionCall::new((
            [0; 20].into(),
            one_eth,
            if let Some(instruction) = instruction {
              Shorthand::Raw(RefundableInInstruction { origin: None, instruction }).encode().into()
            } else {
              vec![].into()
            },
          ))
          .abi_encode()
          .into(),
        };

        *nonce += 1;

        let sig =
          k256::ecdsa::SigningKey::from(k256::elliptic_curve::NonZeroScalar::new(*key).unwrap())
            .sign_prehash_recoverable(tx.signature_hash().as_ref())
            .unwrap();

        let mut bytes = vec![];
        tx.encode_with_signature_fields(&Signature::from(sig), &mut bytes);

You can observe this failure in my CI: https://github.com/serai-dex/serai/actions/runs/11162161674/job/31026214470 (where the commit prior worked, and this commit didn't touch any of our work with Ethereum).

This obviously isn't a MRE and I apologize for that. I just wanted to file this ASAP until I have time to poke more later. I didn't see any filed issues/open PRs recently which seems to be relevant here. Off-hand, I have some loose guess this a quirk with publishing legacy TXs (outside of an envelope) but I truly don't know. https://github.com/serai-dex/serai//blob/55fb8a588922dcae8a13a6f269fc98f9c2843e1b/Cargo.lock#L102-L507 for my alloy versions, which is 0.3 for the non-core libs and not yet 0.4 (which may be of relevance here).

@kayabaNerve kayabaNerve added T-bug Type: bug T-needs-triage Type: this issue needs to be labelled labels Oct 7, 2024
@github-project-automation github-project-automation bot moved this to Todo in Foundry Oct 7, 2024
@yash-atreya
Copy link
Member

This should be fixed by alloy-rs/alloy#1428 which will be available in the next alloy release

@yash-atreya yash-atreya removed the T-needs-triage Type: this issue needs to be labelled label Oct 7, 2024
@klkvr
Copy link
Member

klkvr commented Oct 7, 2024

@yash-atreya alloy-rs/alloy#1428 only fixed into_signed fn. If incorrect signature is passed directly to encode_with_signature_fields (like in this example), this would still result in incorrect encoding. do we want to handle it there instead?

@kayabaNerve
Copy link
Contributor Author

If incorrect signature is passed directly to encode_with_signature_fields (like in this example),

Sorry, what did I do which was incorrect? I'm not saying my code is absolutely 100% correct, yet the above seems without issue to me. I'm not manually specifying any recovery ID value. I also am specifying one (one is returned by sign_prehash_recoverable and is accordingly passed into Signature::from).

This also was accepted by anvil in the past (meaning if this was an invalid encoding, which it sounds like it was, there's also an invalid decoding/validation issue which has yet to be linked here).

@klkvr
Copy link
Member

klkvr commented Oct 7, 2024

If incorrect signature is passed directly to encode_with_signature_fields (like in this example),

Sorry, what did I do which was incorrect? I'm not saying my code is absolutely 100% correct, yet the above seems without issue to me. I'm not manually specifying any recovery ID value. I also am specifying one (one is returned by sign_prehash_recoverable and is accordingly passed into Signature::from).

This also was accepted by anvil in the past (meaning if this was an invalid encoding, which it sounds like it was, there's also an invalid decoding/validation issue which has yet to be linked here).

when converting k256 signature from sign_prehash_recoverable to alloy's signature, RecoveryId is getting converted to a boolean parity https://github.com/alloy-rs/core/blob/172aa98d4c0d20b0f1fe1812a8d378d7005a4011/crates/primitives/src/signature/parity.rs#L28, because we don't know anything about the transaction type when converting it. this boolean value ends up being rlp encoded, resulting in invalid legacy transaction encoding because it is expected to encode as either 27 or 28

this error didn't occur earlier because we've only recently enforced this in alloy during decoding

@kayabaNerve
Copy link
Contributor Author

kayabaNerve commented Oct 7, 2024

Hm. I think I'd label that an issue with encode_with_signature_fields but sounds like it'll be on me to handle (at least for now). Thank you for the context. Sorry that it appears I've opened issues re: alloy in foundry.

@klkvr
Copy link
Member

klkvr commented Oct 27, 2024

Tracking in alloy-rs/alloy#1510, should be fixed soon by alloy-rs/core#776

@klkvr klkvr closed this as completed Oct 27, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
Status: Done
Development

No branches or pull requests

3 participants