test: correct EOS updateauth zero-wait signing vector - #66
BitHighlander wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change set is limited to test vectors/fixtures (no production logic), and the digest update aligns the tests with the intended corrected serialization behavior.
Pull request overview
This PR updates the EOS updateauth signing test fixtures to match the corrected serialization behavior for an authority containing one account and zero waits, and adds an independent (non-emulator) vector test to lock in the expected digest.
Changes:
- Add a standalone unit test that reproduces both the corrected digest and the legacy “phantom wait” digest for comparison.
- Update the existing EOS sign-tx test to assert the corrected transaction digest for the zero-waits case.
File summaries
| File | Description |
|---|---|
| tests/unit/test_eos_updateauth_vector.py | New independent wire-level vector test covering corrected vs legacy digest behavior. |
| tests/test_msg_eos_signtx.py | Updates the expected updateauth digest for the zero-waits authority case (and documents why). |
Review details
Suppressed comments (2)
tests/unit/test_eos_updateauth_vector.py:20
varcurrently uses multiple statements per line, which makes the varuint32 encoding easy to misread (especially around the loop/shift). Reformatting improves readability without changing behavior.
def var(x):
b=bytearray()
while x>=128:b.append((x&127)|128);x>>=7
b.append(x);return bytes(b)
tests/unit/test_eos_updateauth_vector.py:21
- The
authpreimage component is built as one long expression, making it difficult to verify field boundaries (threshold/keys/accounts/waits) when updating vectors. Splitting it into a parenthesized multi-line expression makes the serialized structure easier to review.
auth=struct.pack('<I',1)+b'\x01\x01'+pub+struct.pack('<H',1)+b'\x01'+name('memememememe')+name('active')+struct.pack('<H',1)+b'\0'
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
Changes are isolated to test vectors/expected hashes and appear consistent with the described firmware serialization fix.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/unit/test_eos_updateauth_vector.py:53
- Using a plain loop here makes it harder to see which vector case failed (and
bytes(6)is a bit opaque). Consider usingsubTestand naming the “phantom wait” bytes explicitly so failures are easier to diagnose.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Round 2 body-only suggestion addressed in 728a243: subTest labels distinguish the corrected and legacy cases, and the phantom wait is explicitly encoded as wait_sec=0/weight=0. Both independent digests still pass. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to test vectors/expected digests, with only a small docstring-clarity nit identified.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/unit/test_eos_updateauth_vector.py:5
- The docstring claims the legacy digest difference comes from using
accounts_countinstead ofwaits_count, but the legacy reproduction in this test is implemented by optionally appending a 6-byte zero wait struct (phantom_wait) after the authority bytes. Updating the docstring to describe the mechanism used in this file will make the vector easier to audit and avoid confusion about what bytes are being varied.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Round 3 body-only suggestion addressed in afcc164: the docstring now describes the exact six-byte zero wait and action-length adjustment used by the independent legacy vector. Both digests pass unchanged. No production code changed. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to test vectors/fixtures, internally consistent (including the SLIP-48 authority key path), and add coverage for both corrected and legacy digests without modifying production code.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Review receipt: round 4 review 5145980415 is on current head afcc164, with zero inline findings, no actionable body-only findings and zero unresolved threads. The independent corrected/legacy digest test passes. No more review requests for this accepted EOS-vector head. PR remains unmerged. |
Correct the EOS updateauth signing fixture for an authority with one account and zero waits. The former expected digest includes a nonexistent six-byte zero wait, matching the firmware accounts_count/waits_count bug corrected in firmware PR #635.
An independent serializer reproduces the corrected digest (5938294e...) and the former digest (fb936ef1...) by adding exactly that phantom wait and its action-length adjustment. The public key was independently derived from the existing fixture mnemonic at m/48'/4'/1'/0'/0'. No other fixtures or dependency features change.
Validation: standalone stdlib vector test passes. Full emulator integration will run through the firmware stack after pinning this commit. This dependency PR remains unmerged and targets the frozen audited ce5c1bb baseline, preserving the narrow review surface.