Enforce canonical encoding for Arkade payloads#1082
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR enforces canonical encoding across asset groups and extensions by introducing minimal LEB128 varint decoding, validating serialization integrity, rejecting non-canonical presence bits and opcode encodings, and verifying round-trip consistency through fuzz tests. ChangesCanonical Encoding Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Review — #1082
Verdict: Request changes (protocol-critical scope → requires human sign-off regardless)
Overall Assessment
This PR closes encoding-level malleability in the Arkade OP_RETURN payload. The approach is sound: reject at parse time anything that doesn't round-trip to identical bytes. The canonical varint implementation is correct, the fuzz oracles are the right design, and the invariant ("parse ⇒ re-serialize == original") is well-chosen. Good work.
However, I have findings ranging from a real correctness gap to cross-repo coordination concerns.
🔴 P0 — Correctness Gap
1. extension.go: deserializeVarSlice is duplicated — only one copy is patched
pkg/ark-lib/extension/extension.go:283 has its own deserializeVarSlice that is correctly updated to use varint.ReadCanonical. But extension.go also has serializeVarSlice at line ~273 (on master). The concern is the other direction: there is a separate deserializeVarSlice in pkg/ark-lib/asset/utils.go:83 which is also patched. Good — both copies are patched. ✅ Verified on re-read.
However, are there any other call sites of binary.ReadUvarint remaining in pkg/ark-lib/? A grep for binary.ReadUvarint across pkg/ark-lib/ would confirm no stragglers. If any remain, the canonical invariant is broken — a non-canonical varint at that call site passes through undetected.
Action: Confirm zero remaining calls to binary.ReadUvarint in pkg/ark-lib/. If any exist, they must also be migrated to varint.ReadCanonical.
🟡 P1 — Cross-Repo Coordination
2. TypeScript SDK and .NET SDK do NOT validate canonical encoding
Both ts-sdk (src/extension/asset/utils.ts:readVarUint) and dotnet-sdk (BufferReader.cs:ReadVarInt) accept non-canonical LEB128. Neither validates trailing bytes or minimal push opcodes in extension parsing.
This is acceptable today because:
- Both SDKs produce canonical output (their encoders naturally emit minimal LEB128)
- This PR only tightens arkd's acceptance, not what arkd emits
But it creates a consistency gap:
- If a TS/dotnet SDK consumer builds a payload from raw bytes (not via the SDK's own serializer), the SDK will happily accept non-canonical data that arkd will reject
- Future SDK versions should match arkd's strictness to avoid confusion
Action: File tracking issues on arkade-os/ts-sdk and arkade-os/dotnet-sdk to add canonical varint validation + trailing byte rejection. Not a blocker for this PR, but should be tracked.
🟡 P1 — Edge Case in Canonical Validation
3. varint.go:22-28: Reader position is consumed even on rejection
func ReadCanonical(r *bytes.Reader) (uint64, error) {
before := r.Len()
v, err := binary.ReadUvarint(r)
if err != nil {
return 0, err
}
var buf [binary.MaxVarintLen64]byte
if before-r.Len() != binary.PutUvarint(buf[:], v) {
return 0, ErrNonMinimal
}
return v, nil
}When ErrNonMinimal is returned, the reader position has already been advanced past the non-canonical varint bytes. If any caller attempts error recovery (unlikely today, but defensive concern), the reader state is inconsistent.
This is fine for the current callers (they all abort on error), but worth a doc comment: // On error, the reader position is undefined.
Action: Minor — add a doc note that reader position is undefined on error.
🟢 P2 — Suggestions
4. extension.go:220-224: Ordering of trailing-bytes check vs. push-opcode check
The current order is: check trailing bytes → check script error → check minimal push. This is fine functionally, but the script error check (tokenizer.Err()) should arguably come first, since if the tokenizer is in an error state, tokenizer.Next() may return misleading results.
Current code:
if tokenizer.Next() {
return nil, fmt.Errorf("non-canonical extension: trailing data after payload")
}
if err := tokenizer.Err(); err != nil {
return nil, fmt.Errorf("non-canonical extension: invalid script: %w", err)
}btcd's ScriptTokenizer.Next() returns false on error, so tokenizer.Next() returning true is unambiguous. But tokenizer.Next() returning false could mean either "no more ops" or "error" — the subsequent Err() check disambiguates correctly. ✅ Order is actually correct.
5. asset_group.go:239-243: Presence mask is hardcoded — consider deriving from constants
const definedPresenceMask = maskAssetId | maskControlAsset | maskMetadataThis is correct today. If a new optional field is added to AssetGroup and its mask constant is defined but not added here, the decoder will reject valid payloads. The risk is low (any new field requires touching this function anyway), but a compile-time assertion or comment would help.
Action: Minor — add a comment: // IMPORTANT: update this mask when adding new optional fields
6. Tests are thorough. The fuzz oracles (parse(x) ok ⇒ serialize(parse(x)) == x) are exactly the right pattern for canonicality. The targeted rejection tests cover all new checks. The varint test covers zero, boundary (127/128), max-uint64, and overlong variants. Well done.
🔒 Protocol-Critical Flag
This PR modifies deserialization of Arkade payloads — the wire format that encodes asset groups inside VTXOs. While the changes are defense-hardening (reject → not accept), any bug here could cause:
- Valid on-chain payloads to be rejected (consensus split)
- Arkd nodes on different versions to disagree on payload validity
The PR description states: "arkd already emits canonical bytes, so no legitimate flow is affected. This only tightens acceptance of malformed or third-party input. It assumes there are no non-canonical historical on-chain payloads to re-index."
This assumption must be verified. If there are any historical on-chain payloads with non-canonical encoding, this PR will break re-indexing.
Action (blocking): Confirm the assumption — scan existing on-chain data (or the indexer DB) for payloads that would fail the new checks. A one-liner check: decode all existing payloads through the new code path and verify zero rejections.
Summary of Required Actions
| # | Severity | Action | Blocking? |
|---|---|---|---|
| 1 | P0 | Grep for remaining binary.ReadUvarint calls in pkg/ark-lib/ |
Yes |
| 2 | P1 | File tracking issues for TS/dotnet SDK canonical validation | No |
| 3 | P1 | Doc note on reader position in varint.ReadCanonical |
No |
| 5 | P2 | Comment on definedPresenceMask maintainability |
No |
| 🔒 | Critical | Verify no historical on-chain payloads use non-canonical encoding | Yes |
Requesting changes primarily for the protocol-critical verification (historical payload scan) and the binary.ReadUvarint audit. The code quality is high — this is good hardening work.
🤖 Reviewed by Arkana (arkade-os/arkd code review agent)
pkg/ark-lib has no remaining direct parser use of binary.ReadUvarint; the only real call is inside internal/varint.ReadCanonical, which is the intended wrapper. Both extension and asset varint decode paths are covered by it. |
|
Other issues raised by Arkana can be ignored IMO. |
1 participant
Enforces that the Arkade
OP_RETURNpayload has a single canonical byte encoding, closing encoding-level malleability. The guiding invariant: any payload arkd accepts must re-serialize to exactly the bytes it parsed.Changes
internal/varint.ReadCanonicalrejects non-minimal LEB128, e.g.0x80 0x00; used by the asset and extension decoders.OP_RETURNpush opcode.All checks are O(1) on the decode path, with no re-serialization.
Scope
Encoding canonicality only. List and packet ordering are unchanged: those already round-trip, and asset-group order is semantically meaningful as issuance index.
Testing
parse(x) ok ⇒ serialize(parse(x)) == x; approximately 11M execs each, with no counterexample.Compatibility
arkd already emits canonical bytes, so no legitimate flow is affected. This only tightens acceptance of malformed or third-party input. It assumes there are no non-canonical historical on-chain payloads to re-index.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests