alloy-consensus crate #83

prestwich · 2023-12-15T14:49:14Z

Motivation

Have signature type support the JSON v and yParity formats

Solution

Signature keeps vrs AND an ecdsa::Signature in memory.

PR Checklist

Added Tests
Added Documentation
Breaking changes

Cargo.toml

prestwich · 2023-12-16T18:44:38Z

ty ty i will update here soon

wip: port ser tests feature: Parity::chain_id test: normalize 27/28 fix: properly normalize in recovery fix: serde works now refactor wip: lots of stuff refactor: push signature into core primitives feat: seal feat: seal fix: k256 feature in network feature: rlp for TxLegacy feature: block wip test fix: payload lenght feat: sealable feature: Tx types tests: they pass???? refactor: envelope module refactor: TxType refactor: simpler rlp enc/decode for envelope fix: todo feature: enable sign_transaction feat: some froms and some cleanup lint: clippy fix: test feature fix: network receipt bounds receipt.rs restore providers

gakonst · 2024-01-05T01:55:23Z

crates/signer/src/signer.rs

+        match self.chain_id() {
+            Some(tx_chain_id) => {
+                if tx_chain_id != chain_id {
+                    return Err(crate::Error::TransactionChainIdMismatch {
+                        signer: chain_id,
+                        tx: tx_chain_id,
+                    });
+                }
+            }
+            None => {
+                self.set_chain_id(chain_id);
+            }
+        }


can simplify with

let Some(tx_chain_id) = self.chain_id() { else self.set_chain_Id(chain_id); return Ok(()) };

and pull the error check outside

don't think it makes it any simpler

gakonst · 2024-01-05T01:56:18Z

crates/signer/src/signer.rs

+        let chain_id = self.chain_id();
+        if let Some(chain_id) = chain_id {
+            tx.set_chain_id_checked(chain_id)?;
+        }
+        let mut sig = self.sign_hash(tx.signature_hash()).await?;
+        if let Some(chain_id) = chain_id.or_else(|| tx.chain_id()) {
+            sig = sig.with_chain_id(chain_id);
+        }


gakonst · 2024-01-05T01:58:49Z

crates/signer/src/wallet/private_key.rs

+        tx.chain_id = Some(1);
+        let sig_1 = sign_tx_test(&mut tx, None).await.unwrap();
+        let expected = "c9cf86333bcb065d140032ecaab5d9281bde80f21b9687b3e94161de42d51895727a108a0b8d101465414033c3f705a9c7b826e596766046ee1183dbc8aeaa6825".parse().unwrap();
+        assert_eq!(sig_1, expected);
+        assert_ne!(sig_1, sig_none);
+
+        tx.chain_id = Some(2);
+        let sig_2 = sign_tx_test(&mut tx, None).await.unwrap();
+        assert_ne!(sig_2, sig_1);
+        assert_ne!(sig_2, sig_none);
+
+        // Sets chain ID.
+        tx.chain_id = None;
+        let sig_none_none = sign_tx_test(&mut tx, None).await.unwrap();
+        assert_eq!(sig_none_none, sig_none);
+
+        tx.chain_id = None;
+        let sig_none_1 = sign_tx_test(&mut tx, Some(1)).await.unwrap();
+        assert_eq!(sig_none_1, sig_1);
+
+        tx.chain_id = None;
+        let sig_none_2 = sign_tx_test(&mut tx, Some(2)).await.unwrap();
+        assert_eq!(sig_none_2, sig_2);
+
+        // Errors on mismatch.
+        tx.chain_id = Some(2);
+        let error = sign_tx_test(&mut tx, Some(1)).await.unwrap_err();
+        let expected_error = crate::Error::TransactionChainIdMismatch { signer: 1, tx: 2 };


gakonst · 2024-01-05T01:59:20Z

crates/signer-aws/src/signer.rs

-    #[cfg(TODO)] // TODO: TypedTransaction
-    #[instrument(err)]
-    async fn sign_transaction(&self, tx: &TypedTransaction) -> Result<Signature> {
-        let mut tx_with_chain = tx.clone();
-        let chain_id = tx_with_chain.chain_id().map(|id| id.as_u64()).unwrap_or(self.chain_id);
-        tx_with_chain.set_chain_id(chain_id);
-
-        let sighash = tx_with_chain.sighash();
-        self.sign_digest_with_eip155(sighash, chain_id).await
+        self.sign_digest_inner(hash).await.map_err(alloy_signer::Error::other)


did we lose tx signing on aws signer?

uses default impl just like wallet

gakonst · 2024-01-05T01:59:54Z

crates/signer-trezor/src/signer.rs

    #[inline]
-    async fn sign_transaction(&self, tx: &TypedTransaction) -> Result<Signature> {
+    #[cfg(TODO)]


work in progress

See #100 as a possible solution

gakonst · 2024-01-06T02:05:58Z

Would love if we can use this to remove a bunch of the Reth storage types. Might mean either manual impls of the Compact trait, or derives on newtypes. Either way would simplify things on Reth side. A fast follow to this PR should remove final Ethers usage from Header etc. types from Anvil.

* feat: implement sign_transaction on Trezor * fix: dyn trait upcasting is not stable yet

mattsse

all of this looks reasonable to me.

it's a lot of code, most of it comes from reth directly. imo it's fine (for now) that we have them duplicated, but want to upstream this to reth soonish.

didn't review the trait abstractions too closely, because I think those are still highly unstable, but the approach is fine.

we have a lot of room for follow ups, for example, extracting constants

mattsse · 2024-01-09T19:11:30Z

crates/eips/src/eip4844.rs

as discussed with @prestwich we want to move a lot of these into a alloy-eip-constants crate or something similar, so revm can easily import those and we don't have them in two places

mattsse · 2024-01-09T19:13:20Z

crates/network/src/lib.rs

+pub struct BlockResponse<N: Network> {
+    #[serde(flatten)]
+    header: N::HeaderResponse,
+    transactions: TransactionList<N::TransactionResponse>,


this probably doesn't work for Uncle blocks, so we either need a type for the uncle requests which is just N::HeaderResponse or do some serde magic here, using a dedicated UncleResponse is probably better

gakonst

good w me - let's get this in so we can finish the anvil core transition

gakonst · 2024-01-09T21:00:45Z

Cargo.toml

-k256 = { version = "0.13.2", default-features = false, features = ["ecdsa", "std"] }
-sha2 = { version = "0.10.8", default-features = false, features = ["std"] }
-spki = { version = "0.7.2", default-features = false, features = ["std"] }
+c-kzg = "0.4.0" # for eip-4844


ideally this would be optional but maybe not possible here?

This is the workspace dependencies, but anyway it's unused

gakonst · 2024-01-09T21:01:08Z

Cargo.toml

+tempfile = "3.8"
+
+[patch.crates-io]
+alloy-primitives = { git = "https://github.com/alloy-rs/core" }


can remove?

No, this will need a 0.6 release

gakonst · 2024-01-09T23:18:28Z

crates/eips/src/eip2718.rs

+
+    /// True if the envelope is the legacy variant.
+    fn is_legacy(&self) -> bool {
+        self.type_flag().is_none()


shouldn't this also be for Some(0)?

I guess so, added in 1a240d0

crates/eips/src/eip2930.rs

gakonst · 2024-01-09T23:19:40Z

crates/network/src/transaction/mod.rs

+// TODO: Remove in favor of dyn trait upcasting (1.76+)
+#[doc(hidden)]
+impl<S: 'static> dyn Transaction<Signature = S> {
+    pub fn __downcast_ref<T: std::any::Any>(&self) -> Option<&T> {
+        if std::any::Any::type_id(self) == std::any::TypeId::of::<T>() {
+            unsafe { Some(&*(self as *const _ as *const T)) }
+        } else {
+            None
+        }
+    }
+}


FYI @prestwich this was needed for the trezor signer

crates/network/src/transaction/signed.rs

prestwich added the enhancement New feature or request label Dec 15, 2023

prestwich self-assigned this Dec 15, 2023

prestwich linked an issue Dec 15, 2023 that may be closed by this pull request

[Feature] impl RLP traits for Signature #82

Closed

prestwich changed the title ~~refactor: signature keeps v r s in mem~~ alloy-consensus crate Dec 16, 2023

prestwich commented Dec 16, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

prestwich force-pushed the prestwich/new-signature branch from 726a92b to c7d7de5 Compare December 18, 2023 11:57

mattsse mentioned this pull request Dec 21, 2023

feat: use reth-rpc-types #89

Merged

prestwich force-pushed the prestwich/new-signature branch from 7c1b541 to 87ee3b5 Compare December 24, 2023 20:04

prestwich added 4 commits December 24, 2023 12:06

fix: network

fd383ce

fix: misc fixes for new signature methods

197d88c

feature: custom 2718 error

fa00a37

fix: transaction may not be RLP

2e94cbb

prestwich mentioned this pull request Dec 25, 2023

Should these derive the rlp encodable macro? #57

Closed

prestwich added 15 commits December 25, 2023 12:53

remove log mod

d3afce7

feature: Header, constants, and pure calculations

873887a

refactor: impl Sealable

8cfb573

refactor: move 2718 and add docs

bc874a1

chore: enable lints

356efb4

fix: const fns

ee68856

fix: correct eip2718 network encoding behavior

1a240d0

feature: arbitrary

a957c3b

cleanup: deps and patch

7ea13e5

feature: Receipt trait

ed028fd

fix: impl bloom_cheap

1d02916

feature: generic signature

119b5cb

doc: document signature type

54b83e7

fix: remove encodable bound

b234327

refactor: signabletx trait

5dac547

prestwich and others added 7 commits December 27, 2023 14:37

misc arbitrary cleanup

3b6b188

fix: encode_2718 length

59c4de0

fix: use core git main instead of local

7a666b3

Merge branch 'main' into prestwich/new-signature

3c2e155

chore: clippy, docs

ae063b8

fix: features

a0799e4

fix: cap arbitrary logs

4a70592

DaniPopes mentioned this pull request Jan 4, 2024

fix(primitives): also apply EIP-155 to Parity::Parity alloy-rs/core#476

Merged

3 tasks

DaniPopes added 2 commits January 5, 2024 01:30

chore: use Vec instead of BufMut where possible

7295a49

feat: fix Signer EIP-155, implement sign_transaction

c89a7b9

gakonst reviewed Jan 5, 2024

View reviewed changes

chore: make ledger internal sign tx public

74e8207

Evalir mentioned this pull request Jan 5, 2024

meta(anvil): anvil-core internal types removal foundry-rs/foundry#6715

Closed

13 tasks

DaniPopes mentioned this pull request Jan 5, 2024

feat: implement sign_transaction on Trezor #100

Merged

3 tasks

chore: remove eip1559 duplicate functions

ba60ce9

DaniPopes added 3 commits January 6, 2024 13:33

feat: implement sign_transaction on Trezor (#100)

8a57c67

* feat: implement sign_transaction on Trezor * fix: dyn trait upcasting is not stable yet

Merge branch 'main' into prestwich/new-signature

c70c11c

chore: relax requirements

f19501d

DaniPopes marked this pull request as ready for review January 9, 2024 18:25

DaniPopes requested a review from Evalir as a code owner January 9, 2024 18:25

gakonst requested review from mattsse and gakonst January 9, 2024 19:04

mattsse approved these changes Jan 9, 2024

View reviewed changes

gakonst approved these changes Jan 9, 2024

View reviewed changes

chore: address review

c93a8f7

DaniPopes merged commit bb469e2 into main Jan 10, 2024
16 of 17 checks passed

DaniPopes deleted the prestwich/new-signature branch January 10, 2024 10:38

DaniPopes mentioned this pull request Jan 18, 2024

[Feature] Don't pass B256 by value #126

Closed

mattsse mentioned this pull request Sep 30, 2024

fix: safe match for next base fee #1399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

alloy-consensus crate #83

alloy-consensus crate #83

prestwich commented Dec 15, 2023

prestwich commented Dec 16, 2023

gakonst Jan 5, 2024

DaniPopes Jan 5, 2024 •

edited

Loading

gakonst Jan 5, 2024

gakonst Jan 5, 2024

gakonst Jan 5, 2024

DaniPopes Jan 5, 2024 •

edited

Loading

gakonst Jan 5, 2024

DaniPopes Jan 5, 2024

DaniPopes Jan 5, 2024

gakonst commented Jan 6, 2024 •

edited

Loading

mattsse left a comment

mattsse Jan 9, 2024

mattsse Jan 9, 2024

gakonst left a comment

gakonst Jan 9, 2024

DaniPopes Jan 10, 2024

gakonst Jan 9, 2024

DaniPopes Jan 10, 2024

gakonst Jan 9, 2024

DaniPopes Jan 10, 2024

gakonst Jan 9, 2024

alloy-consensus crate #83

alloy-consensus crate #83

Conversation

prestwich commented Dec 15, 2023

Motivation

Solution

PR Checklist

prestwich commented Dec 16, 2023

Choose a reason for hiding this comment

DaniPopes Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst commented Jan 6, 2024 • edited Loading

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes Jan 5, 2024 •

edited

Loading

DaniPopes Jan 5, 2024 •

edited

Loading

gakonst commented Jan 6, 2024 •

edited

Loading