net: Serve blocks directly from disk when possible
Needs ReviewPublic
Actions

Authored by PiRK on Tue, Nov 18, 11:01.

Details

Reviewers

None

Group Reviewers

Restricted Project

Summary

In ProcessGetBlockData, send the block data directly from disk if
a non-compact block is requested. This is a valid shortcut as the
on-disk format matches the network format.

This is expected to increase performance because a deserialization and
subsequent serialization roundtrip is avoided.

This is a backport of core#13151

Note that this is a very old PR (2018) so we need to include a number of other changes that were ignored in previous backports (or not yet fully backported). Some notable ones are:

Test Plan

ninja all check-all

run on mainnet for a while, check the logs to make sure we are not unexpectedly disconneted by peers receiving our blocks

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 35093
Build 69645: Build Diff
Build 69644: arc lint + arc unit

Event Timeline

PiRK created this revision.Tue, Nov 18, 11:01

Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Nov 18, 11:01

teamcity edited the summary of this revision. (Show Details)Tue, Nov 18, 11:01

PiRK published this revision for review.Tue, Nov 18, 11:15

PiRK edited the summary of this revision. (Show Details)

PiRK added inline comments.

src/net_processing.cpp
3494–3497 ↗	(On Diff #56642)	We could refactor the code a bit further and include this branch in the fast-path as well, but this is not expected to happen very often as it implies a dysfunctional peer requesting cmpctblock when it shouldn't (old blocks). So imo it is better to stick close to the upstream solution.

benchmark, comparing ReadBlockFromDisk and ReadRawBlockFromDisk (from https://github.com/bitcoin/bitcoin/pull/26684/commits):

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        1,446,132.00 |              691.50 |    0.2% |      0.02 | `ReadBlockFromDiskTest`
|           42,053.96 |           23,778.97 |    0.4% |      0.01 | `ReadRawBlockFromDiskTest`

Harbormaster completed remote builds in B35091: Diff 56642.Tue, Nov 18, 11:31

@bot build-ibd

src/net_processing.cpp
3396 ↗	(On Diff #56642)	this could be factored, it's duplicated below except for 1 thing (see below)
3404 ↗	(On Diff #56642)	Is expected to be LogError or LogPrintLevel(BCLog::NET, BCLog::Level::Error, (categorized) as below ? It should be the same

teamcity awarded a token.Tue, Nov 18, 13:14

PiRK planned changes to this revision.Tue, Nov 18, 13:58

PiRK added inline comments.

src/net_processing.cpp
3404 ↗	(On Diff #56642)	It should be LogError both here and below. The below code has an out of order backport issue, `LogError` was not yet available when D18256 was merged. And LogDebug and LogPrint (deprecated) are aliases of each-other, so we can definitely dedup, the behavior is identical.