Skip to content

Sync with the main repo#47

Merged
hebasto merged 328 commits into
bitcoin-core:mainfrom
hebasto:211003-upstream
Oct 3, 2021
Merged

Sync with the main repo#47
hebasto merged 328 commits into
bitcoin-core:mainfrom
hebasto:211003-upstream

Conversation

@hebasto
Copy link
Copy Markdown
Member

@hebasto hebasto commented Oct 3, 2021

Sync with the main repo up to bitcoin-core/gui@446b706

To verify:

$ cd ../gui && git log --oneline > /tmp/gui && cd ../gui-qml && git log --oneline > /tmp/gui-qml && diff -u /tmp/gui /tmp/gui-qml

jamesob and others added 30 commits September 15, 2021 15:46
Only perform certain behavior (namely that related to servicing
the getblocktemplate RPC call) for the active chainstate when
calling UpdateTip.

Co-authored-by: Jon Atack <jon@atack.com>
Instead of (ab)using the existing BLOCK_VALID_* flags to mark CBlockIndex entries which
we haven't yet fully validated (but assume validity for use with UTXO snapshot
loading), introduce a status flag that specifically marks an assumed-valid state.

This state is then removed in RaiseValidity() when the block has actually been
validated.

This distinction will allow us to make the necessary changes to various parts of the
system to facilitate assumeutxo/background chainstate validation but without leaking
details like snapshot height, as we had done previously.

Changes that actually make use of this flag follow in future commits.
Mark the block index entries that are beneath the snapshot base block as
assumed-valid.  Subsequent commits will make use of this flag in other
parts of the system.
Adjust CheckBlockIndex to account for
- assumed-valid block indexes lacking transaction data, and
- setBlockIndexCandidates for the background chainstate not containing certain entries
  which rely on assumed-valid ancestors.
and move `CreateAndActivateUTXOSnapshot()` into it for reuse
in future test modules.
To avoid linker error on some platforms:
    bitcoin/bitcoin#21526 (comment)

Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
This is so we can create blocks within unittests and have them
be processed by specific chainstates (instead of the just the
active one).
This is done by removing an unnecessary loop in Good_() and looping
through the new tables in MakeTried() more efficiently, choosing a
starting value that allow us to stop early in typical cases.

Co-authored-by: John Newbery <john@johnnewbery.com>
After the performance improvement for Good(), the direct method is only 2x faster
as opposed to 60x before.
… makeChain, etc methods

e4709c7 Start using init makeNode, makeChain, etc methods (Russell Yanofsky)

Pull request description:

  Use `interfaces::Init::make*` methods instead of `interfaces::Make*` functions, so interfaces can be constructed differently in different executable without having to change any code. (So for example `bitcoin-gui` can make an `interfaces::Node` pointer that communicates with a `bitcoin-node` subprocess, while `bitcoin-qt` can make an `interfaces::Node` pointer that controls node code in the same process.)

  ---

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.

ACKs for top commit:
  jamesob:
    reACK bitcoin/bitcoin@e4709c7
  achow101:
    ACK e4709c7
  benthecarman:
    utACK e4709c7

Tree-SHA512: 580c1979dbb2ef444157c8e53041e70d15ddeee77e5cbdb34f70b6d228cc2d2fe3843825f172da84e506200c58f7e0932f7cd4c006bb5058c1f4e43259394834
fa22ddc doc: Fix walletdisplayaddress RPC doc (MarcoFalke)

Pull request description:

  Previously the description was empty

ACKs for top commit:
  fanquake:
    ACK fa22ddc

Tree-SHA512: f8898f9545378dd9f68eba036745776c1bc8d43c4753a19fcd9c3d945a11d9e07cd394d9ed215c2d090964830f785f812f18608f04fc7e2fb73730fc77c5d807
It looks like this should have been caught by CI but perhaps there was a
conflict with a recently merged PR. Failure reported by fanquake <fanquake@gmail.com>
bitcoin/bitcoin#22219 (comment)
… versioning scheme

fad8606 doc: Update snap release process for new versioning scheme (MarcoFalke)

Pull request description:

ACKs for top commit:
  jarolrod:
    ACK fad8606
  fanquake:
    ACK fad8606 - moving these instructions to the packaging repo.

Tree-SHA512: 80a9d1484a6544e44a7e57967a62d6e3084efe946de40ac5f0ad0aeb79399d578a7b690186b33d394402d383715741203ebd09ff3f376a5f657045ed96f273e7
865ee1a Fix Qt test broken by #22219 (Russell Yanofsky)

Pull request description:

  It looks like this should have been caught by CI but there might have been a conflict with a recently merged PR like #19101. Failure was reported by fanquake bitcoin/bitcoin#22219 (comment)

  Fix avoids null WalletClient pointer dereference in address book test by adding MakeWalletClient call and making address book test initialization more consistent with wallet test initialization:

  https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/addressbooktests.cpp#L63-L66
  https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/wallettests.cpp#L141-L144

ACKs for top commit:
  fanquake:
    ACK 865ee1a - I'm merging this now to unbreak the build.

Tree-SHA512: 1f32b7fc79fa79fcf8600d23063896cbc7f8bbcff39d95747ecd546e754581f0f36ece3098ddecded175afccbb3709b4232da39a400dda23b7e550f361b515fb
3174425 Cleanup headers after #20788 (Hennadii Stepanov)

Pull request description:

  This is a header cleanup after #20788.

ACKs for top commit:
  vasild:
    ACK 3174425

Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d
This works around an issue when trying to use `std::filesystem::remove_all`
with the ARM GCC on Buster. Has been split out of #20744.

See comments starting here:
bitcoin/bitcoin#20744 (comment).
Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93201.
252d1a7 ci: use Debian Bullseye in ARM CI (fanquake)

Pull request description:

  This works around an issue when trying to use `std::filesystem::remove_all` with the ARM GCC on Buster.  Has been split out of #20744.

  See commentary starting here: bitcoin/bitcoin#20744 (comment).
  Also: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93201.

ACKs for top commit:
  MarcoFalke:
    cr ACK 252d1a7
  hebasto:
    ACK 252d1a7, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: ca71f5cb07fe06c1c7f0160935e667ffeb62bd6a1a89b54124b5633c5c176347a2207aaa5eca68938ed89db9778d357e42b677115d4ed386fa2d7d2ffa5025ad
This is required to use std::filesystem on macOS as support for it only
landed in the libc++ dylib shipped with 10.15.

See also: https://developer.apple.com/documentation/xcode-release-notes/xcode-11-release-notes

Clang now supports the C++17 <filesystem> library for iOS 13, macOS 10.15, watchOS 6, and tvOS 13.
This is done in order to prepare the make_utxo helper to use MiniWallet,
which only supports creating transactions with single inputs, i.e. we
need to create amounts small enough to be funded by coinbase transactions
(50 BTC).
This is done in order to prepare the make_utxo helper to use MiniWallet,
which only supports creating transactions with single inputs, i.e. we
need to create amounts small enough to be funded by coinbase transactions
(50 BTC).
5008dd8 doc: Remove stale comment for CPrivKey (Calvin Kim)

Pull request description:

  Removes stale doc about `secure_allocator` being defined in `allocators.h`.

ACKs for top commit:
  laanwj:
    ACK 5008dd8
  theStack:
    Code-review ACK 5008dd8

Tree-SHA512: eb65aff6db5b27d0db2b86f1d1dc6e066daccdaf00f7f9f95b5bee507167fcea2601316cdbd70da4ba32f1fab1e28e440a7e3cabd7b1a72c07dd20b1367361f0
fad4f44 test: Set peertimeout in write_config (MarcoFalke)

Pull request description:

  This avoids having to remember to set it whenever mocktime is used with
  peer connections. Also, it might help avoiding disconnects when
  attaching a debugger to a running test.

ACKs for top commit:
  laanwj:
    Concept and code review ACK fad4f44

Tree-SHA512: 00c742571c0524c1b3f55e0217433ef7aa2dccccc12650caab98b4cf9231669f37fc589c7475f28d5725ffe2436c76205920eaece4a47fd27dc8872421a48e5c
fa66a7d p2p: Rename fBlocksOnly, Add test (MarcoFalke)
fac66d0 test: Simplify p2p_blocksonly test with new miniwallet rescan_utxos method (MarcoFalke)

Pull request description:

  `fBlocksOnly` has several issues:
  * The name is confusing
  * It is untested

  Fix both.

ACKs for top commit:
  laanwj:
    Code review ACK fa66a7d

Tree-SHA512: 4218f455eeb37297f74603d7d44895288605844ae828a40dfb7a70215f1a058ac5ad945a22732f5ebcad3ad375d54ba360bea69ea79639a30d4c88b042448f0f
…dBlockFromDisk without cs_main lock

350e034 consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack)

Pull request description:

  Commit ccd8ef6 "Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock" in #11281 moved the cs_main lock from caller to `ReadBlockFromDisk()` for calling `CBlockIndex::GetBlockPos()`, but the second invocation doesn't have the lock, and IIUC there is no guarantee the compiler can know if state has changed.

  Use the `blockPos` local variable instead, rename it to `block_pos`, and make it const.

ACKs for top commit:
  laanwj:
    Code review ACK 350e034
  theStack:
    Code-review ACK 350e034
  promag:
    Code review ACK 350e034.

Tree-SHA512: 0df0614ab1876885c85f7b53c604a759a29008da8027e95503b4726d2b820ec6d27546020c613337ff954406e01cb5d191978ba4a12124052fed6e1b0e9a226f
meshcollider and others added 23 commits September 30, 2021 12:06
…enaming

2b90eae doc: update developer docs for subtree renaming (fanquake)

Pull request description:

  Update the developer docs after the [recent subtree renaming](bitcoin/bitcoin#22646 (comment)).

ACKs for top commit:
  hebasto:
    ACK 2b90eae

Tree-SHA512: ed0eec8db888e60595c07f4fad0a506673e4b10345fb2dd6d1a98d785da22bddf1fe8896aa52fd67f5e1688e3c91c6b642739e08646f1b920f50f0d35037d961
489060d qt: Do not show unused widgets at startup (Hennadii Stepanov)

Pull request description:

  On master (8d83f9c), when starting without wallets the `labelWalletEncryptionIcon` and
  `labelWalletHDStatusIcon` widgets are not used but still visible as empty space:

  ![Screenshot from 2021-09-29 21-59-22](https://user-images.githubusercontent.com/32963518/135332107-f02db936-3c3a-436b-9e78-c5721df8852b.png)

  If one opens any wallet then closes it, the widget layout becomes densed:

  ![Screenshot from 2021-09-29 22-05-31](https://user-images.githubusercontent.com/32963518/135332650-83787bc4-fa8e-417e-8d53-a9fdb1c8bfc9.png)

  This PR makes widget layout densed at startup.

  Fixes bitcoin-core#428.

ACKs for top commit:
  jarolrod:
    ACK 489060d
  promag:
    Code review ACK 489060d.

Tree-SHA512: bda7195225ecd203bb3269ebe7fc264aaf7f57b922deb83a04127584a5d6123950741fb431161523e84630927c2f617e85c085bbbe75ad8559da7b2947de1bdd
…during shutdown

451ca24 qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov)
f3a17bb qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov)
b4e0d2c qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov)
332dea2 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov)
c8bae37 qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov)
7fa91e8 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov)
6f6fde3 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov)
59f7ba4 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov)
7830cd0 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov)
13f6188 qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)

Pull request description:

  On master (1ef34ee) during shutdown `QApplication` exits the main event loop, then re-enter again.

  This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for bitcoin-core#59.

  Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO).

  The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent.

  This PR does not change behavior, and all touched dialogs are still application modal.
  As a follow up, a design research could suggest to make some dialogs window modal.

  NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine.

ACKs for top commit:
  laanwj:
    Code review and lighly tested ACK 451ca24
  promag:
    ACK 451ca24, just changed signal to `quitRequested`.

Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
90be29c wallet: enable SQLite extended result codes (Sebastian Falbesoner)

Pull request description:

  With this change, we get more fine-grained error messages if something goes wrong in the course of communicating with the SQLite database. To pick some random examples, the error codes SQLITE_IOERR_NOMEM, SQLITE_IOERR_CORRUPTFS or SQLITE_IOERR_FSYNC are way more specific than just a plain SQLITE_IOERR, and the corresponding error messages generated by sqlite3_errstr() will hence give a better hint to the user (or also to the developers, if an error report is sent) what the cause for a failure is.

  See the SQLite documentation
  https://www.sqlite.org/c3ref/extended_result_codes.html
  https://www.sqlite.org/c3ref/c_abort_rollback.html
  > In its default configuration, SQLite API routines return one of 30 integer result codes. However, experience has shown that many of these result codes are too coarse-grained. They do not provide as much information about problems as programmers might like. In an effort to address this, newer versions of SQLite (version 3.3.8 2006-10-09 and later) include support for additional result codes that provide more detailed information about errors.

ACKs for top commit:
  Sjors:
    utACK 90be29c
  achow101:
    ACK 90be29c
  laanwj:
    Code review ACK 90be29c

Tree-SHA512: 2b7a60860c206f2b5f8ff9d4a7698efdee897c9ad024621b8fd165b841c20746d9780da3cf46aaf448a777e229a5b3cdf3a4792e8ef82cda9c5d46e354a9a598
…nd place comment correctly

8ff3743 Revert "doc: Remove outdated comments" (Hennadii Stepanov)

Pull request description:

  Unfortunately, in #23094 the assumption that #14336 makes comments outdated is wrong. As pointed in  bitcoin/bitcoin#23094 (comment), the #14336 just moved the relevant code a few lines down.

  This PR reverts commit ee7891a, and moves the comments into the right place.

  I apologize about that.

ACKs for top commit:
  MarcoFalke:
    cr ACK 8ff3743
  laanwj:
    ACK 8ff3743

Tree-SHA512: 84aca627bb5b49c06fc172778f9b9407482c5a873ccbc3dc40167e6a8ad0bc60475d6a469c843b7b42712e35cf3fc2d3518923e791d5e0c59628e042acc72747
1d44513 Squashed 'src/crc32c/' changes from b5ef9be675..0d624261ef (MarcoFalke)

Pull request description:

  Only change is a warning fix for arm.

  ```
    CXX      crc32c/src/crc32c_libcrc32c_a-crc32c.o
  In file included from crc32c/src/crc32c.cc:11:0:
  crc32c/src/./crc32c_arm64_check.h: In function ‘bool crc32c::CanUseArm64Crc32()’:
  crc32c/src/./crc32c_arm64_check.h:43:37: warning: the address of ‘long unsigned int getauxval(long unsigned int)’ will never be NULL [-Waddress]
     unsigned long hwcap = (&getauxval != nullptr) ? getauxval(AT_HWCAP) : 0;
                            ~~~~~~~~~~~^~~~~~~~~~

ACKs for top commit:
  laanwj:
    Code review ACK fac1c13
  fanquake:
    ACK fac1c13

Tree-SHA512: 22a52caf67dd89092eff1f075fbf5c5d16bdca9146ba042ce5d3fcc10ce1485e950964089f8536c938ebe650676e03a789d3597fe45b19920fd2c5e72f1391ad
…move getauxval() weak linking

4446ef0 build: remove support for weak linking getauxval() (fanquake)
e56100c build: remove arm includes from getauxval() check (fanquake)

Pull request description:

  It was [pointed out in #23030](bitcoin/bitcoin#23030 (comment)) that we might be able to get rid of our weak linking of [`getauxval()`](https://man7.org/linux/man-pages/man3/getauxval.3.html) (`HAVE_WEAK_GETAUXVAL`) entirely, with only Android being a potential holdout:
  > I wonder if it's time to get rid of HAVE_WEAK_GETAUXVAL. I think it's confusing. Either we build against a C library that has this functionality, or not. We don't do this weak linking thing for any other symbols and recently got rid of the other glibc backwards compatibility stuff.
  > Unless there is still a current platform that really needs it (Android?), I'd prefer to remove it from the build system, it has caused enough issues.

  After looking at Android further, it would seem that given we are moving to using `std::filesystem`, which [requires NDK version 22 and later](https://github.com/android/ndk/wiki/Changelog-r22), and `getauxval` has been available in the since [API version 18](https://developer.android.com/ndk/guides/cpu-features#features_using_libcs_getauxval3), that shouldn't really be an issue. Support for API levels < 19 will be dropped with the NDK 24 release, and according to [one website](https://apilevels.com/), supporting API level 18+ will cover ~99% of devices. Note that in the CI we currently build with NDK version 22 and API level 28.

  The other change in this PR is removing the include of headers for ARM intrinsics, from the check for strong `getauxval()` support in configure, as they shouldn't be needed. Including these headers also meant that the check would basically only succeed when building for ARM. This would be an issue if we remove weak linking, as we wouldn't detect `getauxval()` as supported on other platforms. Note that we also use `getauxval()` in our RNG when it's available.

  I've checked that with these changes we detect support for strong `getauxval()` on Alpine (muslibc). On Linux, previously we'd be detecting support for weak getauxval(), now we detect strong support. Note that we already require glibc 2.17, and `getauxval()` was introduced in `2.16`.

  This is an alternative / supersedes #23030.

ACKs for top commit:
  laanwj:
    Code review and tested ACK 4446ef0

Tree-SHA512: 5f2a9e9cc2d63bddab73f0dcb169d4d6beda74622af82bc0439722f1189f81d052e2fc1eaf27056a7a606320d5ddc4c11075f0d051dd93d77c5e1c15337f354a
…r multiple lines in the log file

2222c04 log: Adjust coin selection log string (MarcoFalke)
fa6c1e8 test: Fix typos in tests (MarcoFalke)
faeae29 log: Avoid broken DEBUG_LOCKORDER log (MarcoFalke)
faffaa8 log: Avoid broken SELECTCOINS log (MarcoFalke)

Pull request description:

  Follow up to commit d8b4b30

ACKs for top commit:
  laanwj:
    re-ACK 2222c04
  practicalswift:
    cr ACK 2222c04

Tree-SHA512: e0daf76815a1b7c4898ceffedeaf7ede093223abf709874f9a0d78c8e41551c14e8b56d055c8fdf06ec698df64e67dfc168bbd8716131b23648d1d1294fa6636
…cale independent std::from_chars(…) (C++17) instead of locale dependent strto{l,ll,ul,ull}

4747db8 util: Introduce ToIntegral<T>(const std::string&) for locale independent parsing using std::from_chars(…) (C++17) (practicalswift)

Pull request description:

  Make `Parse{Int,UInt}{32,64}` use locale independent `std::from_chars(…)` (C++17) instead of locale dependent `strto{l,ll,ul,ull}`.

  [About `std::from_chars`](https://en.cppreference.com/w/cpp/utility/from_chars): _"Unlike other parsing functions in C++ and C libraries, `std::from_chars` is locale-independent, non-allocating, and non-throwing."_

ACKs for top commit:
  laanwj:
    Code review ACK 4747db8

Tree-SHA512: 40f2cd582bc19ddcf2c498eca3379167619eff6aa047bbac2f73b8fd8ecaefe5947c66700a189f83848751f9f8c05645e83afd4a44a1679062aee5440dba880a
… main GUI thread

2fe69ef qt, wallet: Drop no longer used WalletController::getOpenWallets() (Hennadii Stepanov)
f6991cb qt, wallet: Add LoadWalletsActivity class (Hennadii Stepanov)
4a024fc qt, wallet, refactor: Move connection to QObject::deleteLater to ctor (Hennadii Stepanov)
f9b633e qt, wallet: Move activity progress dialog from data member to local (Hennadii Stepanov)

Pull request description:

  This PR improves the GUI responsiveness during initial wallets loading at startup (especially ones that have tons of txs), and shows a standard progress dialog for long loading:

  ![DeepinScreenshot_select-area_20210522230626](https://user-images.githubusercontent.com/32963518/119239625-0b3a9380-bb53-11eb-9a54-34980d8a1194.png)

  Fixes bitcoin-core#247.

ACKs for top commit:
  ryanofsky:
    Code review ACK 2fe69ef. Just suggested changes since last review: squashing commits and dropping unused method (thanks!)
  shaavan:
    reACK 2fe69ef
  promag:
    Code review ACK 2fe69ef.

Tree-SHA512: 2ac3cb48886e0005fc36b3fd0c2b35abd557186be16db3145d753c34d94188e4f4ff14dc07fb0fb7558944f84498204a3988f8284fd56c6d85b47bc9081e71a6
- update call to round() with satoshi_round() to avoid intermittent test failures
- rename fee_per_kB to feerate_BTC_kvB for precision
- store division result in feerate_BTC_vB
dc3ec74 Add rescan removal release note (Samuel Dobson)
bccd1d9 Remove -rescan startup parameter (Samuel Dobson)
f963b0f Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson)
6c00649 Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson)

Pull request description:

  Remove the `-rescan` startup parameter.

  Rescans can be run with the `rescanblockchain` RPC.

  Rescans are still done on wallet-load if needed due to corruption, for example.

ACKs for top commit:
  achow101:
    ACK dc3ec74
  laanwj:
    re-ACK dc3ec74

Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c
-BEGIN VERIFY SCRIPT-
git grep -l 'RESCAN_REQUIRED' src | xargs sed -i 's/RESCAN_REQUIRED/NEED_RESCAN/g'
-END VERIFY SCRIPT-
…ED to NEED_RESCAN

8615507 scripted-diff: rename DBErrors::RESCAN_REQUIRED to NEED_RESCAN (Samuel Dobson)

Pull request description:

  Suggested here as a trivial follow-up: bitcoin/bitcoin#23123 (comment)

  Makes RESCAN_REQUIRED consistent with NEED_REWRITE

ACKs for top commit:
  achow101:
    ACK 8615507
  jonatack:
    ACK 8615507

Tree-SHA512: 82d057c45e192cd6dd8a47675b52699e6cbc82272609a971e6e5d6796aad14a941a70e40d3913dbb611f79c8eadff8030c60ea6f203f2edc3720c0e78c166b97
…cks in blocks-only mode

18c5b23 [test] Test that -blocksonly nodes still serve compact blocks. (Niklas Gögge)
a79ad65 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections. (Niklas Gögge)
5e231c1 [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection. (Niklas Gögge)
5bf6587 [test] Test that -blocksonly nodes do not request high bandwidth mode. (Niklas Gögge)
0dc8bf5 [net processing] Dont request compact blocks in blocks-only mode (Niklas Gögge)

Pull request description:

  A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks.

  In both high- and low-bandwidth relaying the `cmpctblock` message is sent. This represent a bandwidth overhead for blocks-only nodes because the `cmpctblock` message is several times larger in the average case than the equivalent `headers` or `inv` announcement.

  ![compact blocks](https://raw.githubusercontent.com/bitcoin/bips/master/bip-0152/protocol-flow.png)

  >**Example:**
  >A block with 2000 txs results in a `cmpctblock` with 2000*6 bytes in short ids. This is several times larger than the equivalent 82 bytes for a `headers` message or 37 bytes for an `inv`.

  ## Approach

  This PR makes blocks-only nodes always use the legacy relaying to download new blocks.
  It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of `sendcmpct(1)`. Additionally a blocks-only node will never request a compact block using `getdata(CMPCT)`.

  A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode.

ACKs for top commit:
  naumenkogs:
    ACK 18c5b23
  rajarshimaitra:
    tACK bitcoin/bitcoin@18c5b23
  jnewbery:
    reACK 18c5b23
  theStack:
    re-ACK 18c5b23 🥛

Tree-SHA512: 0c78804aa397513d41f97fe314efb815efcd852d452dd903df9d4749280cd3faaa010fa9b51d7d5168b8a77e08c8ab0a491ecdbdb3202f2e9cd5137cddc74624
… v1.x

6e1150e fuzz: add guide to fuzzing with Eclipser v1.x (Alex Groce)

Pull request description:

  MarcoFalke and practicalswift here's an Eclipser guide, reconstructed from their documentation and my docker history getting it up and running.  It might be good if someone confirmed it actually works for them in a fresh ubuntu 20.04.

ACKs for top commit:
  practicalswift:
    ACK 6e1150e

Tree-SHA512: ca855932fd7a2c1d1005d572ab5fabc26f42d779f9baf279783f08a43dd72ec60f57239135d30c2a82781e593626fec2c96bb19fb91e1b777cef2d83a54eba35
…sserting

0ab4c3b Return false on corrupt tx rather than asserting (Samuel Dobson)

Pull request description:

  Takes up #19793

  Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth `assert`ing.

ACKs for top commit:
  achow101:
    ACK 0ab4c3b
  laanwj:
    Code review ACK 0ab4c3b
  ryanofsky:
    Code review ACK 0ab4c3b. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this.

Tree-SHA512: 4a1a412e7c473d176c4e09123b85f390a6b0ea195e78d28ebd50b13814b7852f8225a172511a2efb6affb555b11bd4e667c19eb8c78b060c5444b62f0fae5f7a
…n the functional test framework

b658d7d test: update assert_fee_amount() in test_framework/util.py (Jon Atack)

Pull request description:

  Follow-up to 42e1b5d (#12486).
  - update call to `round()` with our utility function `satoshi_round()` to avoid intermittent test failures
  - rename `fee_per_kB` to `feerate_BTC_kvB` for precision
  - store division result in `feerate_BTC_vB`

  Possibly resolves #19418.

ACKs for top commit:
  meshcollider:
    utACK b658d7d

Tree-SHA512: f124ded98c913f98782dc047a85a05d3fdf5f0585041fa81129be562138f6261ec1bd9ee2af89729028277e75b591b0a7ad50244016c2b2fa935c6e400523183
fad5a18 doc: Combine 23.0 release notes (MarcoFalke)

Pull request description:

  `doc/release-notes-remove-rescan.md` didn't include a `(#pull_nr)`, so fix that and also combine the notes while touching them.

ACKs for top commit:
  shaavan:
    ACK fad5a18
  fanquake:
    ACK fad5a18

Tree-SHA512: 1528619f056197d410d0fa5415742ae303fd21b236f220451614f9ec07668a4f34a76298907378b2c4b25a405cff8cd1d205024d2f3d8f207d11900a607f765f
@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Oct 3, 2021

cc @promag @jarolrod

Copy link
Copy Markdown
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK ac29328, parent commits are correct.

@hebasto
Copy link
Copy Markdown
Member Author

hebasto commented Oct 3, 2021

A script to verify this PR has been added to the OP.

@hebasto hebasto merged commit ac29328 into bitcoin-core:main Oct 3, 2021
@hebasto hebasto deleted the 211003-upstream branch October 3, 2021 14:42
Copy link
Copy Markdown
Contributor

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post-merge ACK, verified correctness of sync

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.