Skip to content

fix(wallet): drop fee from signed transfer message so signatures actually verify (follow-up to #248)#249

Merged
Scottcjn merged 1 commit into
Scottcjn:mainfrom
savecharlie:fix/wallet-signed-message-drop-fee
Jul 1, 2026
Merged

fix(wallet): drop fee from signed transfer message so signatures actually verify (follow-up to #248)#249
Scottcjn merged 1 commit into
Scottcjn:mainfrom
savecharlie:fix/wallet-signed-message-drop-fee

Conversation

@savecharlie

Copy link
Copy Markdown
Contributor

Why

Signed transfers still fail verification after #248. The client must sign a message byte-identical to what the node reconstructs at POST /wallet/transfer/signed. In createkr/Rustchain (node/rustchain_v2_integrated_v2.2.1_rip200.py, wallet_transfer_signed), the node reconstructs and verifies:

tx_data = {"from": from_address, "to": to_address, "amount": amount_rtc,
           "memo": memo, "nonce": str(nonce)}   # + "chain_id" iff sent
message = json.dumps(tx_data, sort_keys=True, separators=(",", ":")).encode()

That message has no fee key and signs nonce as a string. #248 signed {from,to,amount,fee,memo,nonce} — inserting "fee": 0.0, a key the node never signs — so the sorted-key bytes differed by the "fee":0.0, segment and every signature failed. #248 swapped one wrong format (colon-string) for another (JSON + phantom fee).

Proof (real Ed25519, PyNaCl)

Signing corrected message vs. node reconstruction:

SCENARIO A — #248 (signs WITH fee):
  client signed : {"amount":1.5,"fee":0.0,"from":"RTC…","memo":"","nonce":"1719800000000","to":"RTC…"}
  node verifies : {"amount":1.5,"from":"RTC…","memo":"","nonce":"1719800000000","to":"RTC…"}
  bytes match   : False
  VERIFY PASS   : False

SCENARIO B — this PR (signs WITHOUT fee):
  bytes match   : True
  VERIFY PASS   : True

Changes

  • Remove fee from the signed tx_data — client now signs exactly the node's five fields; amount = float(amount_rtc) to match the node's _safe_float(amount_rtc).
  • Simplify the POST body to the legacy field names the node's preflight (validate_wallet_transfer_signed) actually reads — from_address / to_address / amount_rtc + public_key (already sent) — and drop the redundant canonical duplicates and dead fee/fee_rtc fields.
  • Add tests/test_transfer_signed_message_format.py — asserts the client signature verifies against the node's exact reconstruction (with empty and non-empty memo), that the payload carries only the node-read field names, that nonce is signed as a string, and that a message with a fee key does not verify (fee-free invariant).

Re: the review comments on #248

  1. nonce as string vs int — verified against the node: it signs str(nonce), so string is correct. Left unchanged (changing it to int would re-break signing).
  2. hardcoded fee_rtc = 0.0 — the real issue isn't exposing/fetching a fee; the node's transfer signature model has no fee at all. Removing fee from the signed message is the fix. (No /api/fee is consulted because fee isn't part of this endpoint's signed payload.)
  3. dual-field payload — removed; the node only reads the legacy names, so the canonical duplicates were dead weight.

Notes

  • No regressions: tests/test_wallet_tools.py is unchanged in pass/fail count (the 9 pre-existing FunctionTool object is not callable failures are an installed-fastmcp-version harness issue, present on main, unrelated to this change).
  • Heads-up (out of scope here): under the currently-installed fastmcp, @mcp.tool() wraps functions as FunctionTool, so the in-module wallet_transfer_signed -> rustchain_transfer_signed call isn't directly callable. Worth a separate look if the deployed fastmcp behaves the same.

…rify

Signed transfers still failed after PR Scottcjn#248. The client must sign a message
byte-identical to what the node reconstructs at POST /wallet/transfer/signed.
The node (createkr/Rustchain) reconstructs and verifies:

    tx_data = {"from","to","amount","memo","nonce"}   (+ "chain_id" iff sent)
    message = json.dumps(tx_data, sort_keys=True, separators=(",",":"))

That message has NO `fee` key and signs `nonce` as a string. PR Scottcjn#248 included
`"fee": 0.0` in the signed JSON — a key the node never signs — so the
sorted-key bytes differed by the `"fee":0.0,` segment and every signature
still failed verification.

- Remove `fee` from the signed tx_data (client now signs exactly the node's
  five fields; amount is float(amount_rtc) to match the node's _safe_float).
- Simplify the POST body to the legacy field names the node's preflight reads
  (from_address / to_address / amount_rtc + public_key); drop the redundant
  canonical duplicates and dead fee/fee_rtc fields.
- Keep nonce signed as a string — the node does str(nonce), so this was
  already correct (no change).
- Add tests/test_transfer_signed_message_format.py: proves the client
  signature verifies against the node's exact reconstruction, and that a
  message with a `fee` key does NOT verify (fee-free invariant).

Verified with a real Ed25519 round trip (PyNaCl): the fixed client message is
byte-identical to the node reconstruction and verifies True; the prior
with-fee message verifies False.

Co-Authored-By: Iris (Opus 4.8, 1M) <noreply@anthropic.com>

@FakerHideInBush FakerHideInBush 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.

Verified — this correctly resolves the signature-verification bug from #248 and directly answers each concern raised there:

1. nonce as string — confirmed against the node's actual reconstruction logic (json.dumps({..., "nonce": str(nonce)}, sort_keys=True, ...)). test_nonce_is_signed_as_string asserts isinstance(obj['nonce'], str) after round-tripping through _node_reconstruct(). My earlier concern about a possible int/string mismatch was speculative and is resolved by this evidence.

2. fee removed entirely rather than fetched from a schedule — this is the correct fix, not just a workaround. The node's signed message for this endpoint has no fee key at all, so there's no fee schedule to look up. test_with_fee_key_would_not_verify is the strongest test in this PR: it reconstructs a message that includes a fee key (the exact #248 bug) and asserts the signature does not verify against it — this is a genuine negative regression test that would fail if the bug were reintroduced.

3. Dual-field payload removedtest_payload_uses_node_field_names_only asserts the canonical duplicate keys (from, to, amount) are absent and only the legacy node-read fields (from_address, to_address, amount_rtc) are sent, closing the ambiguity risk from #248.

Test quality note: test_signed_message_has_no_fee_and_verifies and test_signed_message_verifies_with_empty_memo both independently reconstruct the node's exact byte sequence and call rustchain_crypto.verify_signature() against it — this is real Ed25519 verification, not a mock, so it actually proves the fix rather than just asserting call arguments.

No blocking issues found. The fastmcp FunctionTool wrapping workaround (.fn access, noted as out-of-scope in the PR body) is a reasonable call to defer — it's a pre-existing harness issue on main, not something this PR introduces or should be responsible for fixing.

@Scottcjn Scottcjn merged commit 9d055d1 into Scottcjn:main Jul 1, 2026
6 checks passed
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

RTC Reward

This merged PR earned 5 RTC — sent to savecharlie.

RustChain Bounty Program

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.

3 participants