test: Detect truncated download in get_previous

maflcko commented at 12:39 pm on December 10, 2025: member

Without this, and end-of-stream is not detected and will just lead to an immediate exit, instead of a re-try.

E.g. https://github.com/bitcoin/bitcoin/actions/runs/20089133013/job/57633839315?pr=34038#step:12:201:

0...
1Downloading: [##--------------------------------------] 5.4%
2Downloading: [##--------------------------------------] 5.4%
3Downloading: [##--------------------------------------] 5.5%
4Downloading: [##--------------------------------------] 5.6%
5Checksum dd02eab18f9154604e38135ef3f98fd310ba3c748074aeb83a71118cd2cd1367 did not match
6Error: Process completed with exit code 1.

Also, remove the 0 fallback value, because if the fallback was ever hit, the program would fail anyway with division by zero error.

test: Detect truncated download in get_previous_releases.py fa75480c84

DrahtBot added the label Tests on Dec 10, 2025

DrahtBot commented at 12:39 pm on December 10, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34040.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, Sjors, l0rinc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Sjors commented at 1:49 pm on December 10, 2025: member

Concept ACK

rkrux approved

rkrux commented at 1:58 pm on December 10, 2025: contributor

Looks fine, ACK fa75480c84ffecc856c2d76b1143b14ebce85d0b

I can’t think of a way in which this change would be problematic. Opening the file in wb mode seems ok as any existing content would be erased in case of a retry after partial download as shown in the PR description.

I checked that all the 5 functional tests using the previous releases have passed, the failing test mining_getblocktemplate_longpoll.py could be unrelated?

DrahtBot added the label CI failed on Dec 10, 2025

DrahtBot requested review from Sjors on Dec 10, 2025

DrahtBot removed the label CI failed on Dec 10, 2025

in test/get_previous_releases.py:137 in fa75480c84

133@@ -134,6 +134,9 @@ def progress_hook(progress_bytes, total_size):
134                 progress_bytes += len(chunk)
135                 progress_hook(progress_bytes, total_size)
136 
137+        if progress_bytes < total_size:

l0rinc commented at 2:47 pm on December 10, 2025:

we could be stricter and make it !=

maflcko commented at 4:39 pm on December 10, 2025:

will leave as-is for now

maflcko commented at 10:15 am on December 11, 2025:

Actually, progress_bytes < total_size: could be considered stricter, because if the server starts replying with the wrong payload size, or appends data, it seems like this could be useful to know via a checksum error.

in test/get_previous_releases.py:125 in fa75480c84

121@@ -122,7 +122,7 @@ def progress_hook(progress_bytes, total_size):
122         if response.status != 200:
123             raise RuntimeError(f"HTTP request failed with status code: {response.status}")
124 
125-        total_size = int(response.getheader('Content-Length', 0))
126+        total_size = int(response.getheader("Content-Length"))

l0rinc commented at 2:52 pm on December 10, 2025:

If missing, this would fail with TypeError, right?

nit to minimize diff:

0        total_size = int(response.getheader('Content-Length'))

maflcko commented at 4:39 pm on December 10, 2025:

black prefers this, so I’ll leave this as-is for now

in test/get_previous_releases.py:132 in fa75480c84

l0rinc commented at 2:55 pm on December 10, 2025:

this is a bit un-pythonic, the walrus could come to the rescue:

0        with open(archive, 'wb') as file:
1            while chunk := response.read(8192):
2                file.write(chunk)
3                progress_bytes += len(chunk)
4                progress_hook(progress_bytes, total_size)

But why is chunking needed in the first place, is it for detailed progress reporting only? If we don’t actually need it (i.e. response.read() works), we could still check the Path(archive).stat().st_size size after? (I haven’t checked locally)

If we do need manual chunks, can we make it a bit more realistic, like: while chunk := response.read(1024 * 1024):?

maflcko commented at 4:37 pm on December 10, 2025:

I am not touching those lines, so I’ll leave this as-is for now

Sjors commented at 3:05 pm on December 10, 2025: member

utACK fa75480c84ffecc856c2d76b1143b14ebce85d0b

l0rinc approved

l0rinc commented at 3:06 pm on December 10, 2025: contributor

code review ACK fa75480c84ffecc856c2d76b1143b14ebce85d0b

Left a few comments, also fine with merging as-is.

fanquake merged this on Dec 10, 2025

fanquake closed this on Dec 10, 2025

maflcko deleted the branch on Dec 11, 2025

test: Detect truncated download in get_previous_releases.py #34040

Code Coverage & Benchmarks

Reviews