net: Serve blocks directly from disk when possible
AcceptedPublic
Actions

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

Details

Reviewers

Fabien

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

Branch

pr13151

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 35091
Build 69641: Build Diff	build-without-wallet · build-diff · build-clang-tidy · build-debug · build-clang · lint-circular-dependencies
Build 69640: 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	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	this could be factored, it's duplicated below except for 1 thing (see below)
3404	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	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.

deduplicate handling of block read error

PiRK added a child revision: D18957: bench: add readblock benchmark.Tue, Nov 18, 15:51

Fabien accepted this revision.Tue, Nov 18, 20:56

This revision is now accepted and ready to land.Tue, Nov 18, 20:56

Harbormaster completed remote builds in B35093: Diff 56648.Tue, Nov 18, 20:57

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Tail of the build log:

-- Performing Test have_C__Wno_duplicated_branches - Failed
-- Performing Test USE_ASM_X86_64
-- Performing Test USE_ASM_X86_64 - Success
-- Found Event component event: /usr/lib/x86_64-linux-gnu/libevent.so
-- Found Event: /usr/include (found suitable version "2.1.12-stable", minimum required is "2.1.8") found components: event 
-- Found Boost component headers: /usr/include (found suitable version "1.74.0", minimum required is "1.64")
-- Found Event component pthreads: /usr/lib/x86_64-linux-gnu/libevent_pthreads.so
-- Found Event: /usr/include (found suitable version "2.1.12-stable", minimum required is "2.1.8") found components: pthreads 
-- Found MiniUPnPc component miniupnpc: /usr/lib/x86_64-linux-gnu/libminiupnpc.so
-- Found MiniUPnPc: /usr/include/miniupnpc (found suitable version "2.2.4", minimum required is "1.9")  
-- Found NATPMP component natpmp: /usr/lib/x86_64-linux-gnu/libnatpmp.so
-- Found NATPMP: /usr/include   
-- Performing Test fuzz_target_builds_without_main_fuzz
-- Performing Test fuzz_target_builds_without_main_fuzz - Failed
-- Found BerkeleyDB component CXX: /usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so
-- Found BerkeleyDB: /usr/include (found suitable version "5.3.28", minimum required is "5.3") found components: CXX 
-- Found SQLite3: /usr/include (found suitable version "3.40.1", minimum required is "3.7.17") 
-- Found ZeroMQ component zmq: /usr/lib/x86_64-linux-gnu/libzmq.so
-- Found ZeroMQ: /usr/include (found suitable version "4.3.4", minimum required is "4.1.5")  
-- Could NOT find Protobuf (missing: Protobuf_DIR)
-- Found Protobuf: /usr/lib/x86_64-linux-gnu/libprotobuf.so (found version "3.21.12") 
-- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "3.0.17")  
-- Looking for EVP_MD_CTX_new
-- Looking for EVP_MD_CTX_new - found
-- Found QREncode component qrencode: /usr/lib/x86_64-linux-gnu/libqrencode.so
-- Found QREncode: /usr/include   
[1/9] Creating directories for 'corrosion-populate'
[1/9] Performing download step (git clone) for 'corrosion-populate'
Cloning into 'corrosion-src'...
remote: Internal Server Error
fatal: unable to access 'https://github.com/corrosion-rs/corrosion.git/': The requested URL returned error: 500
Cloning into 'corrosion-src'...
remote: Internal Server Error
fatal: unable to access 'https://github.com/corrosion-rs/corrosion.git/': The requested URL returned error: 500
Cloning into 'corrosion-src'...
remote: Internal Server Error
fatal: unable to access 'https://github.com/corrosion-rs/corrosion.git/': The requested URL returned error: 500
-- Had to git clone more than once: 3 times.
CMake Error at corrosion-subbuild/corrosion-populate-prefix/tmp/corrosion-populate-gitclone.cmake:39 (message):
  Failed to clone repository: 'https://github.com/corrosion-rs/corrosion.git'


FAILED: corrosion-populate-prefix/src/corrosion-populate-stamp/corrosion-populate-download /work/abc-ci-builds/build-clang-tidy/_deps/corrosion-subbuild/corrosion-populate-prefix/src/corrosion-populate-stamp/corrosion-populate-download 
cd /work/abc-ci-builds/build-clang-tidy/_deps && /usr/bin/cmake -P /work/abc-ci-builds/build-clang-tidy/_deps/corrosion-subbuild/corrosion-populate-prefix/tmp/corrosion-populate-gitclone.cmake && /usr/bin/cmake -E touch /work/abc-ci-builds/build-clang-tidy/_deps/corrosion-subbuild/corrosion-populate-prefix/src/corrosion-populate-stamp/corrosion-populate-download
ninja: build stopped: subcommand failed.

CMake Error at /usr/share/cmake-3.25/Modules/FetchContent.cmake:1616 (message):
  Build step for corrosion failed: 1
Call Stack (most recent call first):
  /usr/share/cmake-3.25/Modules/FetchContent.cmake:1756:EVAL:2 (__FetchContent_directPopulate)
  /usr/share/cmake-3.25/Modules/FetchContent.cmake:1756 (cmake_language)
  /usr/share/cmake-3.25/Modules/FetchContent.cmake:1970 (FetchContent_Populate)
  cmake/modules/RustHelper.cmake:9 (FetchContent_MakeAvailable)
  chronik/CMakeLists.txt:6 (include)


-- Configuring incomplete, errors occurred!
See also "/work/abc-ci-builds/build-clang-tidy/CMakeFiles/CMakeOutput.log".
See also "/work/abc-ci-builds/build-clang-tidy/CMakeFiles/CMakeError.log".
Build build-clang-tidy failed with exit code 1