fix: use correct varint decoding for Handshake

[agaffney]

• move varint decode/encode from messages to common place
• add additional block tests
• use custom varint decode in block/transaction decode

Fixes #438

---

Summary by cubic

Fixes incorrect varint (CompactSize) decoding for Handshake by introducing a shared implementation and using it across blocks, transactions, and protocol messages. Adds new block tests and hash checks to prevent regressions. Fixes #438.

• Bug Fixes

  • Use ReadUvarintReader for tx input/output counts and witness lengths in block/transaction decoding.
  • Update message decoders to use the same varint logic for list sizes.

• Refactors

  • Move varint encode/decode to internal/handshake/varint.go and replace per-file helpers with WriteUvarint/ReadUvarint.

^{Written for commit b5e0442. Summary will update automatically on new commits.}

Summary by CodeRabbit

• Refactor

  • Centralized varint encoding/decoding into a single, consistent implementation and updated block, transaction, and protocol message paths to use it.
  • Standardized message encode/decode flows and improved related error handling.

• Tests

  • Reworked handshake tests to a table-driven format consolidating test cases and improving coverage and maintainability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This change adds a new Handshake-specific varint implementation in internal/handshake/varint.go (ReadUvarint, ReadUvarintReader, WriteUvarint) implementing prefix-byte compact encoding and little-endian multi-byte forms. Call sites previously using binary.ReadUvarint or internal helpers were updated to use the new exported functions in block decoding, transaction decoding (including witness parsing), and protocol message encode/decode paths. The messages package removed internal uvarint helpers and now uses the exported functions. Tests for handshake block decoding were refactored to a table-driven format.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• internal/handshake/varint.go: verify correctness of prefix handling, little-endian reads/writes, and error messages.
• internal/handshake/transaction.go: review witness size reads and loop boundaries where binary.ReadUvarint was replaced.
• internal/handshake/block.go: confirm block transaction count decoding now uses ReadUvarintReader consistently.
• internal/handshake/protocol/messages.go: ensure all Encode/Decode paths switched to exported ReadUvarint/WriteUvarint and removed helpers didn't change behavior.
• internal/handshake/block_test.go: confirm table-driven tests cover previous cases and correctly validate headers, tx presence, and hashes.

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: use correct varint decoding for Handshake' directly addresses the main change: correcting varint decoding logic in the handshake package.
Linked Issues check	✅ Passed	The PR addresses issue #438 by implementing correct varint decoding for witness sizes and transaction counts in block/transaction decoders, which resolves the reported failure.
Out of Scope Changes check	✅ Passed	All changes are scoped to varint handling in the handshake package: new varint.go file, updates to block/transaction/message decoders, and test additions for regression prevention.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/handshake-varint

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cubic-dev-ai]

No issues found across 5 files

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

internal/handshake/protocol/messages.go (1)

309-335: Fix make() calls: convert uint64 count to int in all message decoders

The decoded count from handshake.ReadUvarint is uint64, but make() requires an int length parameter. This causes a compile-time type error in all four message decoders:

• MsgAddr.Decode (line 327): m.Peers = make([]NetAddress, count)
• MsgGetData.Decode (line 359): m.Inventory = make([]InvItem, count)
• MsgGetHeaders.Decode (line 393): m.Locator = make([][32]byte, count)
• MsgHeaders.Decode (line 424): m.Headers = make([]*handshake.BlockHeader, count)

The for i := range count loops are valid under Go 1.22, but allocations must use an int-typed value.

Convert count to int after the payload-length validation in each method:

func (m *MsgAddr) Decode(data []byte) error {
	count, bytesRead, err := handshake.ReadUvarint(data)
	if err != nil {
		return err
	}
	data = data[bytesRead:]
	if len(data) != int(count*netAddressLength) { // nolint:gosec
		return errors.New("invalid payload length")
	}
+	peerCount := int(count)
-	m.Peers = make([]NetAddress, count)
+	m.Peers = make([]NetAddress, peerCount)
-	for i := range count {
+	for i := range peerCount {
		if err := m.Peers[i].Decode(data[:netAddressLength]); err != nil {
			return err
		}
		data = data[netAddressLength:]
	}
	return nil
}

Apply the same pattern to MsgGetData, MsgGetHeaders, and MsgHeaders with appropriate variable names (invCount, locCount, hdrCount).

internal/handshake/block_test.go (1)

18-61: The review comment is accurate and identifies a real critical compilation error.

Verification findings:

1. decodeHex returns []byte — confirmed at lines 18-20
2. Invalid type conversion syntax — The code attempts [32]byte(decodeHex(...)) at lines 36, 41, 49, 54, 96, 115, and [24]byte(decodeHex(...)) at line 46. In Go, slice-to-array conversion via cast is not valid syntax; this will fail compilation with "cannot convert" errors.
3. Struct fields expect arrays — Confirmed from internal/handshake/block.go:
   • PrevBlock, NameRoot, WitnessRoot, MerkleRoot are [32]byte
   • ExtraNonce is [24]byte
   • Outpoint.Hash is [32]byte
4. No helpers exist — No bytes32, bytes24, or similar conversion helpers found in the codebase
5. Proposed solution is idiomatic — Using copy() helpers is the standard Go pattern for this scenario

Conclusion: The code will not compile without the suggested fix. The review comment's recommendation to add bytes32() and bytes24() helper functions is correct and necessary.

🧹 Nitpick comments (1)

internal/handshake/varint.go (1)

17-69: Varint implementation matches CompactSize; consider adding focused encode/decode tests

ReadUvarintReader / ReadUvarint and WriteUvarint correctly implement the expected CompactSize-style varint layout (1/3/5/9-byte forms with 0xfd/0xfe/0xff prefixes) and centralized error handling for truncated multi-byte values. This is exactly what the block/transaction/message decoders needed.

Given how central this utility now is, it would be worthwhile to add small, direct unit tests here (round-trips for boundary values like 0xfc, 0xfd, math.MaxUint16, math.MaxUint32, and a large uint64), so regressions in varint handling are caught closer to the source.

Also applies to: 72-91

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c383bee and b5e0442.

📒 Files selected for processing (5)

• internal/handshake/block.go (1 hunks)
• internal/handshake/block_test.go (1 hunks)
• internal/handshake/protocol/messages.go (8 hunks)
• internal/handshake/transaction.go (3 hunks)
• internal/handshake/varint.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

internal/handshake/protocol/messages.go (1)

internal/handshake/varint.go (2)

• WriteUvarint (72-91)
• ReadUvarint (17-27)

internal/handshake/block.go (1)

internal/handshake/varint.go (1)

• ReadUvarintReader (29-70)

internal/handshake/transaction.go (1)

internal/handshake/varint.go (1)

• ReadUvarintReader (29-70)

🔇 Additional comments (2)

internal/handshake/block.go (1)

48-58: Switch to shared CompactSize reader is correct and aligns with Handshake encoding

Using ReadUvarintReader(r) here fixes the earlier misuse of binary.ReadUvarint (LEB128) for Handshake’s CompactSize varints and centralizes the logic with the other decode paths. The surrounding loop and error handling remain appropriate.

internal/handshake/transaction.go (1)

52-75: The code is correct for Go 1.24—no changes needed

Go 1.22 and later (including the target Go 1.24.0) allow make() to accept any integer type, including uint64, without explicit conversion. The current code compiles without error:

• i.Witness = make([][]byte, witnessCount) is valid (witnessCount is uint64)
• for j := range witnessCount is valid (Go 1.22+ supports range over integers)
• i.Witness[j] = make([]byte, itemLength) is valid (itemLength is uint64)

No type conversion is required.

Likely an incorrect or invalid review comment.

[agaffney]

@cubic-dev-ai please review this

[cubic-dev-ai]

@cubic-dev-ai please review this

@agaffney I've started the AI code review. It'll take a few minutes to complete.

[cubic-dev-ai]

No issues found across 5 files