Skip to content

state: Recover sender address from transaction signature - #1615

Open
chfast wants to merge 2 commits into
masterfrom
state/tx-recover-sender
Open

state: Recover sender address from transaction signature#1615
chfast wants to merge 2 commits into
masterfrom
state/tx-recover-sender

Conversation

@chfast

@chfast chfast commented Jul 27, 2026

Copy link
Copy Markdown
Member

No description provided.

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.69231% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.41%. Comparing base (d8508b9) to head (52c648f).

Files with missing lines Patch % Lines
test/statetest/statetest_runner.cpp 60.00% 3 Missing and 3 partials ⚠️
test/state/errors.hpp 0.00% 1 Missing and 1 partial ⚠️
test/unittests/state_rlp_decode_test.cpp 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1615      +/-   ##
==========================================
- Coverage   97.46%   97.41%   -0.06%     
==========================================
  Files         170      170              
  Lines       15402    15446      +44     
  Branches     3604     3615      +11     
==========================================
+ Hits        15012    15047      +35     
- Misses        282      287       +5     
- Partials      108      112       +4     
Flag Coverage Δ
eest-develop 88.13% <71.42%> (-0.21%) ⬇️
eest-develop-gmp 25.78% <48.07%> (+<0.01%) ⬆️
eest-legacy 17.32% <0.00%> (-0.05%) ⬇️
eest-libsecp256k1 27.91% <48.07%> (-0.01%) ⬇️
eest-stable 88.10% <71.42%> (-0.21%) ⬇️
evmone-unittests 92.94% <69.23%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
core 95.96% <90.00%> (-0.03%) ⬇️
tooling 90.13% <60.00%> (-0.30%) ⬇️
tests 99.79% <94.11%> (-0.02%) ⬇️
Files with missing lines Coverage Δ
test/state/transaction.cpp 100.00% <100.00%> (ø)
test/state/transaction.hpp 100.00% <ø> (ø)
test/unittests/state_rlp_decode_test.cpp 99.51% <94.11%> (-0.24%) ⬇️
test/state/errors.hpp 79.01% <0.00%> (-2.01%) ⬇️
test/statetest/statetest_runner.cpp 79.59% <60.00%> (-6.13%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chfast
chfast force-pushed the state/tx-recover-sender branch from 47981b6 to d75a382 Compare July 27, 2026 08:30
@chfast chfast changed the title state: Remover sender address from transaction signature state: Recover sender address from transaction signature Jul 27, 2026
@chfast
chfast force-pushed the state/tx-recover-sender branch 2 times, most recently from a3e2e0c to 21a09df Compare July 27, 2026 14:10
@chfast
chfast requested a review from Copilot July 27, 2026 14:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds sender (signer) address recovery from transaction signatures in the state-test harness, aligning transaction execution with node-style behavior rather than trusting JSON-provided senders.

Changes:

  • Introduces state::recover_sender() to recover the signer address from (v, r, s) and the transaction’s serialized bytes.
  • Updates the state test runner to recover sender from txbytes and report INVALID_SIGNATURE when recovery fails.
  • Adds unit tests covering legacy protected/unprotected v handling and signature s-range rejection; adds a new error code/message for invalid signatures.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/unittests/state_rlp_decode_test.cpp Adds unit tests for sender recovery and invalid-s rejection.
test/statetest/statetest_runner.cpp Uses signature-based sender recovery when txbytes is present; maps failures to INVALID_SIGNATURE.
test/state/transaction.hpp Declares recover_sender() and documents signature validity expectations/limitations.
test/state/transaction.cpp Implements recover_sender() by slicing the signing preimage from the original RLP bytes and calling secp256k1 recovery.
test/state/errors.hpp Adds INVALID_SIGNATURE error code and message.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/state/transaction.cpp Outdated
Comment on lines +151 to +162
const auto typed = tx.type != Transaction::Type::legacy;
auto payload = txbytes.substr(typed ? 1 : 0); // Skip the EIP-2718 type byte.

rlp::Header header;
[[maybe_unused]] const auto header_decoded = rlp::decode_header(payload, header);
assert(header_decoded); // tx has been decoded from txbytes, so its list header is valid.

// The signature fields are canonically encoded, so their sizes locate the slice boundary.
const auto signature_size =
rlp::encode(tx.v).size() + rlp::encode(tx.r).size() + rlp::encode(tx.s).size();
assert(signature_size <= header.payload_length);
auto preimage = bytes{payload.substr(0, header.payload_length - signature_size)};
Comment thread test/state/transaction.hpp Outdated
Comment on lines +91 to +101
/// Recovers the sender (the signer) of the transaction @p tx decoded from @p txbytes,
/// std::nullopt if the signature is invalid: r or s outside [1, secp256k1n), or s in the upper
/// half (EIP-2).
///
/// The serialization is needed as well because the signing preimage is a slice of it; @p tx must
/// be what decode_transaction(@p txbytes) returned.
/// TODO: The rules are applied at every revision, but EIP-2 (low s) starts at Homestead and
/// EIP-155 (v carrying the chain id) at Spurious Dragon, so a pre-Homestead transaction with a
/// high s is rejected here and a pre-EIP-155 one with v >= 35 is accepted.
[[nodiscard]] std::optional<address> recover_sender(
const Transaction& tx, bytes_view txbytes) noexcept;
The state test runner still took the sender from the fixture's template,
so a signature was only ever checked for shape. The 39 remaining
frontier/validation/bad_v_r_s cases of execution-specs tests@v20.0.1 are
legacy transactions that decode cleanly and carry an out-of-range r or s;
nothing rejected them.

Add state::recover_sender() and use it, as a node does; a signature that
does not recover makes the transaction invalid (INVALID_SIGNATURE).
Recovery is strict, so EIP-2 low-s and r, s in [1, secp256k1n) come from
ecrecover itself.

The signing preimage is a slice of the serialization -- the payload
without the trailing (v, r, s), and for a protected legacy transaction
(chain_id, 0, 0) in their place -- so recover_sender() takes the decoded
transaction together with the bytes it came from, and finds the end of
the signed prefix by subtracting the sizes of the canonically encoded
signature fields. Reusing the slice avoids restating every transaction
type's field order next to rlp_encode(), which already states it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

recover_sender() read the transaction's list header by hand where
rlp::take_list_payload() does it -- and checks that it is a list, which
the open-coded version did not. It also picked the EIP-155 or the
pre-EIP-155 base to subtract from v before taking the parity; both bases
are odd, so the parity of v alone decides it either way.

In the runner the transaction's emptiness and the error code held the same
fact, kept in sync by resetting the optional; an engaged error code now
carries it alone. The JSON template the transaction starts from is built
only when the test has no encoding to decode it from, which for the EEST
fixtures is never.

The two recovery tests share the decode-then-recover helper.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants