Skip to content

[F-26] fix(network): remove dead double-read in refresh_ipl() OP_FOUND proof#137

Merged
adequatelimited merged 1 commit intoaudit-fixesfrom
audit/F-26-refresh-ipl-double-read
Apr 5, 2026
Merged

[F-26] fix(network): remove dead double-read in refresh_ipl() OP_FOUND proof#137
adequatelimited merged 1 commit intoaudit-fixesfrom
audit/F-26-refresh-ipl-double-read

Conversation

@adequatelimited
Copy link
Copy Markdown
Collaborator

Closes #114

Summary

refresh_ipl() sends an OP_FOUND message to help lagging peers discover a heavier chain. The proof payload was malformed due to a dead double-read: the first read_tfile() result was immediately cleared and overwritten by a second read with different parameters, but the packet length was set from the first read's count. The receiver would reject the mismatched proof, silently breaking the background peer-help mechanism.

Change

Removed the dead first read block and redundant memset calls. Replaced with a single read_tfile() matching send_found()'s proven pattern:

// Before — dead code, mismatched length/content, wrong offset
memset(tx.buffer, 0, sizeof(tx.buffer));
if (sub64(Cblocknum, CL64_32(NTFTX), bnum)) memset(bnum, 0, 8);
count = read_tfile(tx.buffer, bnum, NTFTX, "tfile.dat");
memset(tx.buffer, 0, sizeof(tx.buffer));
if (read_tfile(tx.buffer, Cblocknum, 54, "tfile.dat") == 0) goto FAIL;

// After — single read, correct offset, consistent with send_found()
if (sub64(Cblocknum, CL64_32(NTFTX - 1), bnum)) memset(bnum, 0, 8);
count = read_tfile(tx.buffer, bnum, NTFTX, "tfile.dat");
if (count != NTFTX) goto FAIL;

The NTFTX - 1 offset produces an inclusive range of 54 trailers ending at Cblocknum, matching what the receiver's contention() expects when validating the proof against the advertised weight (stamped by send_tx() from the global Weight).

Added count != NTFTX check to skip the send on short or failed reads — the receiver would reject incomplete proof anyway, so this avoids a wasted network round trip.

Added a TODO comment in send_found() noting the same check should be added there.

Testing

  • Clean build with -Wall -Werror -Wextra -Wpedantic

Closes #114

refresh_ipl() performed two sequential read_tfile() calls into
tx.buffer. The first read's output was immediately cleared and
overwritten by the second, but count from the first read was used
to set the packet length — making the OP_FOUND proof malformed
(length from read A, buffer content from read B). Receiving peers
would reject the mismatched proof, silently breaking the background
peer-help mechanism.

Replaced with a single read_tfile() matching send_found()'s pattern,
including the NTFTX-1 offset for correct inclusive range. Added
count != NTFTX check to skip the send on short/failed reads.
Added TODO comment in send_found() for the same check.
@adequatelimited
Copy link
Copy Markdown
Collaborator Author

For send_found() triggered by refresh_ipl() trying to help lagging peers realize they are behind the network, we were zeroing the proof buffer after filling it. Essentially telling the lagging peer, "you're behind, here's the proof" and sending them 54 block trailers worth of all 0s. One of the more amusing oversights in the history of the codebase, now corrected, with significant network performance improvements now anticipated.

@adequatelimited adequatelimited merged commit 2c67f7f into audit-fixes Apr 5, 2026
4 of 5 checks passed
@adequatelimited adequatelimited deleted the audit/F-26-refresh-ipl-double-read branch April 13, 2026 03:24
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.

1 participant