fix(rpc): version-immune payload-0 CBOR decode (tolerate CheckPack pad)#45
fix(rpc): version-immune payload-0 CBOR decode (tolerate CheckPack pad)#45DHEBP wants to merge 1 commit into
Conversation
Decode exactly the FIRST CBOR item in Arguments.UnmarshalBinary and ignore trailing bytes. CheckPack pads every payload-0 to its size limit with random trailing bytes; fxamacker/cbor v2.4.0's Unmarshal tolerated that pad, but v2.7+ returns ExtraneousDataError on remaining bytes. A streaming Decoder reads a single item and stops — version-agnostic, no dependency pin. Without this, a cbor bump past ~v2.7 silently orphans all payload-0 messaging.
Cherry-pick of DEROFDN#44 onto transmission-alpha-base so the Transmission protocol pin carries all advertised correctness fixes (DEROFDN#20, DEROFDN#44, DEROFDN#45, proof-hardening).
moralpriest
left a comment
There was a problem hiding this comment.
Approve the approach
Switching dec.Unmarshal(data, &localmap) → dec.NewDecoder(bytes.NewReader(data)).Decode(&localmap) is the right version-agnostic fix. CheckPack pads random bytes after the CBOR map, and streaming Decode reads one item and stops, so the pad is ignored by construction. This keeps the DecMode options (e.g. TimeTagRequired) and changes nothing for valid payloads on the currently vendored cbor. LGTM.
Nit 1 — breaking version is v2.5.0, not v2.7
The code comment and PR description say v2.7+ / "v2.4.0 tolerated, v2.7+ hard error". The actual change shipped in v2.5.0:
- fxamacker/cbor v2.5.0, PR #380:
Unmarshal/Validnow returnExtraneousDataErroron remaining bytes. UnmarshalFirst(the pad-tolerant whole-buffer API) also landed in v2.5.0 (PR #398).
Please update the wording to v2.5.0+ so the next person doing a dependency bump targets the right release.
Nit 2 — add a round-trip test (recommended, optional)
There's no test covering CheckPack(limit) → UnmarshalBinary. Since this path turns into a silent landmine on any cbor ≥ v2.5, a small rpc test would lock the contract:
- Build
Arguments(e.g.RPC_DESTINATION_PORT+RPC_COMMENT). CheckPack(limit)withlimit >raw CBOR size → assertlen(packed) == limitandlen(packed) >rawMarshalBinarysize.UnmarshalBinary(packed)succeeds and values match (Has/Value).- Exact-size pack (
limit ==raw size) still decodes. - Empty/
nildata errors without panic.
Using a fixed limit constant in the test keeps rpc free of a transaction import.
Merge order (process note)
Land this before or together with any cbor bump to v2.5+ (e.g. PR #50, which vendors cbor v2.5.0). Without this fix on the wire, every padded payload-0 decode returns ExtraneousDataError and wallet payload-0 messaging breaks.
Thanks for the fix — just the version wording and (ideally) a test and it's ready.
CheckPack appends random padding after the payload-0 CBOR blob. Decoding the whole buffer used to work on older CBOR decoders that ignored trailing bytes; v2.7+ treats the pad as a hard error, so payload decode fails with no obvious signal. Decode one CBOR item and stop — pad is ignored by construction, so the decode path survives a library bump. No behavior change for valid payloads on current libs.