fa9ca13f35be0a023aeed78775ad66f95717b28b refactor: Sort includes of touched source files (MarcoFalke)
facb152697b8d7b75a9e6108f8896f774b06b35f scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7227594e2f59499cf7f7f9420284e04 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
achow101:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
janb84:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
stickies-v:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 doc: Improve m_best_header documentation (Martin Zumsande)
ee673b9aa0157d94220722491f135aef23142521 validation: remove m_failed_blocks (Martin Zumsande)
ed764ea2b4edb3cf1925a4bff5f39567a8be54ac validation: Add more checks to CheckBlockIndex() (Martin Zumsande)
9a70883002e1fee76c24810808af4fb43f2c8cf5 validation: in invalidateblock, calculate m_best_header right away (Martin Zumsande)
8e39f2d20d09592035ae048d0cfe955c733310d9 validation: in invalidateblock, mark children as invalid right away (Martin Zumsande)
4c29326183ba3c9d0b198cb2cec37c3119862c19 validation: cache all headers with enough PoW in invalidateblock (Martin Zumsande)
15fa5b5a908d1019fc1b3042901b42bee0a1bd95 validation: call InvalidBlockFound also from AcceptBlock (Martin Zumsande)
Pull request description:
Some fields in validation are set opportunistically by "best effort":
- The `BLOCK_FAILED_CHILD` status (which means that the block index has an invalid predecessor)
- `m_best_header` (the most-work header not known to be invalid).
This means that there are known situations in which these fields are not set when they should be, or set to wrong values. This is tolerated because the fields are not used for anything consensus-critical and triggering these situations involved creating invalid blocks with valid PoW header, so would have a cost attached. Also, having stricter guarantees for these fields requires iterating over the entire block index, which has some DoS potential, especially with any header above the checkpoint being accepted int he past (see e.g. #11531).
However, there are reasons to change this now:
- RPCs use these fields and can report wrong results
- There is the constant possibility that someone could add code that expects these fields to be correct, especially because it is not well documented that these fields cannot always be relied upon.
- DoS concerns have become less of an issue after #25717 - now an attacker would need to invest much more work because they can't fork off the last checkpoint anymore
This PR continues the work from #30666 to ensure that `BLOCK_FAILED_CHILD` status and `m_best_header` are always correct:
- it adds a call to `InvalidChainFound()` in `AcceptBlock()`.
- it adds checks for `BLOCK_FAILED_CHILD` and `m_best_header` to `CheckBlockIndex()`. In order to be able to do this, the existing cache in the RPC-only `InvalidateBlock()` is adjusted to handle these as well. These are performance optimizations with the goal of avoiding having a call of `InvalidChainFound()` / looping over the block index after each disconnected block.
I also wrote a fuzz test to find possible edge cases violating `CheckBlockIndex`, which I will PR separately soon.
- it removes the `m_failed_blocks` set, which was a heuristic necessary when we couldn't be sure if a given block index had an invalid predecessor or not. Now that we have that guarantee, the set is no longer needed.
ACKs for top commit:
stickies-v:
re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
achow101:
reACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
ryanofsky:
Code review ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1 with only minor code & comment updates
TheCharlatan:
Re-ACK f6b782f3aad4a6bcf823a9a0fabb4418bca1eea1
Tree-SHA512: 1bee324216eeee6af401abdb683abd098b18212833f9600dbc0a46244e634cb0e6f2a320c937a5675a12af7ec4a7d10fabc1db9e9bc0d9d0712e6e6ca72d084f
239fc4d62e73511b3ef5117706d4c5131a921955 doc, windows: CompanyName "Bitcoin" => "Bitcoin Core project" (Hodlinator)
Pull request description:
Brings Windows executables in line with */share/setup.nsi.in:14* used by the installer.
Discovered while reviewing tangential PR: https://github.com/bitcoin/bitcoin/pull/32634#discussion_r2112641918
ACKs for top commit:
maflcko:
lgtm ACK 239fc4d62e73511b3ef5117706d4c5131a921955
Sjors:
utACK 239fc4d62e73511b3ef5117706d4c5131a921955
janb84:
utACK 239fc4d62e73511b3ef5117706d4c5131a921955
hebasto:
ACK 239fc4d62e73511b3ef5117706d4c5131a921955.
Tree-SHA512: 5855e78c32e15a1e4e9b1a6bdefd29c45676a64b3eb4470cb98fa0eea02701edadbde7153143757b525e9a66eb3b49bbba926e8e322307ae6ea4a44ac23eeffb
e98c51fcce9ae3f441a416cab32a5c85756c6c64 doc: update tor.md to mention the new -proxy=addr:port=tor (Vasil Dimov)
ca5781e23a8f299ff4f143d2355218f551e65944 config: allow setting -proxy per network (Vasil Dimov)
Pull request description:
`-proxy=addr:port` specifies the proxy for all networks (except I2P). Previously only the Tor proxy could have been specified separately via `-onion=addr:port`.
Make it possible to specify separately the proxy for IPv4, IPv6, Tor and CJDNS by e.g. `-proxy=addr:port=ipv6`. Or remove the proxy for a given network, e.g. `-proxy=0=cjdns`.
Resolves: https://github.com/bitcoin/bitcoin/issues/24450
ACKs for top commit:
pinheadmz:
ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
caesrcd:
reACK e98c51fcce
danielabrozzoni:
Code Review ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
1440000bytes:
ACK e98c51fcce
Tree-SHA512: 0cb590cb72b9393cc36357e8bd7861514ec4c5bc044a154e59601420b1fd6240f336ab538ed138bc769fca3d17e03725d56de382666420dc0787895d5bfec131
b44514b876333a94ae242da8b1e4cee439c2d37e rpc, doc: update `listdescriptors` RCP help (rkrux)
Pull request description:
This RPC lists all the descriptors present in the wallet, not only the ones that were imported, but also the ones generated when a new wallet is created.
It can be verified by creating a new wallet and calling the `listdescriptors` RPC, which will contain 8 ranged descriptors that are created for every new wallet.
Also, update the description to get rid of "descriptor-enabled" because this is the only wallet type available now after removal of legacy wallets.
ACKs for top commit:
maflcko:
lgtm ACK b44514b876333a94ae242da8b1e4cee439c2d37e
achow101:
ACK b44514b876333a94ae242da8b1e4cee439c2d37e
pablomartin4btc:
ACK b44514b876333a94ae242da8b1e4cee439c2d37e
theStack:
ACK b44514b876333a94ae242da8b1e4cee439c2d37e
Tree-SHA512: d1018dd42fc4de12793f3e4f3be79ecb3fdee46fbc93ec8adb62b29a86e74aba2605d9908632107061f48ef8ee6f39ef6d0e34cc5e91acd93bc02242a2cee3eb
a189d636184b1c28fa4a325b56c1fab8f44527b1 add release note for datacarriersize default change (Greg Sanders)
a141e1bf501bb2660f3a62083a65678250085e56 Add more OP_RETURN mempool acceptance functional tests (Peter Todd)
0b4048c73385166144d0b3e76beb9a2ac4cc1eca datacarrier: deprecate startup arguments for future removal (Greg Sanders)
63091b79e70b8e230a122fa6fb3dac91c80638e7 test: remove unnecessary -datacarriersize args from tests (Greg Sanders)
9f36962b07eff2369577a17c8adeaa0433697e1c policy: uncap datacarrier by default (Greg Sanders)
Pull request description:
Retains the `-datacarrier*` args, marks them as deprecated, and does not require another startup argument for multiple OP_RETURN outputs.
If a user has set `-datacarriersize` the value is "budgeted" across all seen OP_RETURN output scriptPubKeys. In other words the total script bytes stays the same, but can be spread across any number of outputs. This is done to not introduce an additional argument to support multiple outputs.
I do not advise people use the option with custom arguments and it is marked as deprecated to not mislead as a promise to offer it forever. The argument itself can be removed in some future release to clean up the code and minimize footguns for users.
ACKs for top commit:
stickies-v:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Sjors:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
polespinasa:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
hodlinator:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
ajtowns:
reACK a189d636184b1c28fa4a325b56c1fab8f44527b1
mzumsande:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
petertodd:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
theStack:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
1440000bytes:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
willcl-ark:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
dergoegge:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
fanquake:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
murchandamus:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
darosior:
Concept ACK a189d636184b1c28fa4a325b56c1fab8f44527b1.
Tree-SHA512: 3da2f1ef2f50884d4da7e50df2121bf175cb826edaa14ba7c3068a6d5b2a70beb426edc55d50338ee1d9686b9f74fdf9e10d30fb26a023a718dd82fa1e77b038
This RPC lists all the descriptors present in the wallet, not only
the ones that were imported, but also the ones generated when a
new wallet is created.
It can be verified by creating a new wallet and calling the
`listdescriptors` RPC, which will contain 8 ranged descriptors that
are created for every new wallet.
Also, update the description to get rid of "descriptor-enabled"
because this is the only wallet type available now after removal of
legacy wallets.
12ff4be9c724c752fe67835419be3ff4d0e65990 test: ensure -rpcallowip is compatible with RFC4193 (Matthew Zipkin)
c02bd3c1875abd877a0dc73fb8866c883b7fcd32 config: Explain RFC4193 and CJDNS interaction in help and init error (Matthew Zipkin)
f728b6b11100fae1e27f7a0ef92a5930fa8cffb3 init: Configure reachable networks before we start the RPC server (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/32433
`MaybeFlipIPv6toCJDNS()` relies on `g_reachable_nets` to distinguish between CJDNS addresses and other IPv6 addresses. In particular, [RFC4193](https://www.rfc-editor.org/rfc/rfc4193#section-3.1) address or "Unique Local Address" with the L-bit unset also begins with the `fc` prefix. #32433 highlights a use case for these addresses that have nothing to do with CJDNS.
On master we don't parse init flags like `-cjdnsreachable` until *after* the HTTP server has started, causing conflicts with `-rpcallowip` because CJDNS doesn't support subnets.
This PR ensures that `NET_CJDNS` is only present in the reachable networks list if set by `-cjdnsreachable` *before* `-rpcallowip` is checked. If it is set all `fc` addresses are assumed to be CJDNS, can not have subnets, and can't be set for `-rpcallowip`.
I also noted this specific parameter interaction in the init help as well as the error message if configured incorrectly.
This can be tested locally:
`bitcoind -regtest -rpcallowip=fc00:dead:beef::/64 -rpcuser=u -rpcpassword=p`
On master this will just throw an error that doesn't even mention IPv6 at all.
On the branch, this will succeed and can be tested by adding the ULA to a local interface.
On linux: `sudo ip -6 addr add fc00:dead:beef::1/64 dev lo`
On macos: `sudo ifconfig lo0 inet6 fc00:dead:beef::1/128 add`
then: `curl -v -g -6 --interface fc00:dead:beef::1 u:p@[::1]:18443 --data '{"method":"getblockcount"}'`
If the `rpcallowip` option is removed, the RPC request will fail to authorize.
Finally, adding `-cjdnsreachable` to the start up command will throw an error and specify the incompatibility:
> RFC4193 is allowed only if -cjdnsreachable=0.
ACKs for top commit:
achow101:
ACK 12ff4be9c724c752fe67835419be3ff4d0e65990
tapcrafter:
tACK 12ff4be9c724c752fe67835419be3ff4d0e65990
ryanofsky:
Code review ACK 12ff4be9c724c752fe67835419be3ff4d0e65990
willcl-ark:
ACK 12ff4be9c724c752fe67835419be3ff4d0e65990
Tree-SHA512: a4dd70ca2bb9f6ec2c0a9463fd73985d1ed80552c674a9067ac9a86662d1c018cc275ba757cebb2993c5f3971ecf4778b95d35fe7a7178fb41b1d18b601c9960
After changes in previous commits, we now mark all blocks that descend from an invalid block
immediately as the block is found invalid. This happens both in the AcceptBlock
and ConnectBlock stages of block validation.
As a result, the pindexPrev->nStatus check in AcceptBlockHeader is now sufficient to detect
invalid blocks and checking m_failed_blocks there is no longer necessary.
This adds checks that
1) Descendants of invalid block indexes are also marked invalid
2) m_best_header cannot be invalid, and there can be no valid
block with more work than it.
Before, m_best_header would be calculated only after disconnecting
multiple blocks, letting go of cs_main in the meantime.
This is in preparation for adding checks to CheckBlockIndex()
requiring that m_best_header is the most-work header not known to be invalid.
Co-authored-by: stringintech <stringintech@gmail.com>
Before, they would be marked as invalid only after disconnecting
multiple blocks, letting go of cs_main in the meantime.
This is in preparation for adding a check to
CheckBlockIndex() requiring that descendants of invalid block indexes
are always marked as invalid.
Entries from highpow_outofchain_headers are now only removed if made invalid,
no longer after inserting into setBlockIndexCandidates, because they
might still become invalid later in the second case.
This means that blocks could be inserted multiple times now into
setBlockIndexCandidates - this won't have any effect, so
behavior isn't changed.
We now include blocks without HaveNumChainTxs() / lower validation status
than BLOCK_VALID_TRANSACTIONS. These checks are still performed at the
spot where we use the cache to insert into setBlockIndexCandidates.
This is in preparation for using the cache for more things than
just setBlockIndexCandidates candidates in the following commits.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
When a block it found invalid during acceptance (but before connection)
we now mark its descendants with BLOCK_FAILED_CHILD and update
m_best_header when these things weren't done reliably before.
This does not fix a serious bug because the flags and m_best_header were being set on a best-effort basis before
and not used for anything critical.
Setting these reliably has a slight performance cost (iterating over the
entire block index) but leads to more consistency in validation and allows removing m_failed_blocks in a later commit.
This can only be triggered by providing a block with sufficient PoW
that is otherwise invalid, so it is not a DoS vector.
cfc42ae5b7ef6d3892907cfe36dc3ae923495d37 fuzz: add a target for the coins database (Antoine Poinsot)
46e14630f7fe8e2d51adc18a4f50345eb26970c7 fuzz: move the coins_view target's body into a standalone function (Antoine Poinsot)
56d878c4650cc46ce54d1d79db054ac44b486605 fuzz: avoid underflow in coins_view target (Antoine Poinsot)
Pull request description:
This reopens https://github.com/bitcoin/bitcoin/pull/28216.
The current `coins_view` target only tests `CCoinsViewCache` using a basic `CCoinsView` instance. The addition of the `coins_view_db` target enables testing with an actual `CCoinsViewDB` as the backend.
ACKs for top commit:
maflcko:
lgtm ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
l0rinc:
code review ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
TheCharlatan:
ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
Tree-SHA512: d3a92f122629f075767453a1abd9819a1c9716db53b997418993fef62d27683324740d0a8f84df76d8a7a45e508ccadeb69553b6f69e29a1238cd7c0be5276ca
f98e1aaf34e347088caa54403521e3b5cb55dd40 rpc: Note in fundrawtransaction doc, fee rate is for package (benthecarman)
Pull request description:
Accidentally made some transactions with a much higher fee rate than I wanted because I did not know this would do it for the package rather than the individual tx.
ACKs for top commit:
achow101:
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
rkrux:
re-ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
danielabrozzoni:
ACK f98e1aaf34e347088caa54403521e3b5cb55dd40
Tree-SHA512: 9f961de1200803ec4d1c6901fd606bb6cf707ffd03942d9dc0d4b6554c827075f99d693b93e892f728679d67e63e12c71da4426dab091b3311d1605bc37251a2
83bfe1485c37d407de7eed11b8ad769a05f78b66 build: add -Wthread-safety-pointer (fanquake)
240a4fb95d5b843826081807019cd405aa654e2b Squashed 'src/leveldb/' changes from 113db4962b..aba469ad6a (fanquake)
Pull request description:
This will become available in Clang 21:
> ThreadSafetyAnalysis now supports -Wthread-safety-pointer, which
> enables warning on passing or returning pointers to guarded variables
> as function arguments or return value respectively. Note that
> ThreadSafetyAnalysis still does not perform alias analysis. The
> feature will be default-enabled with -Wthread-safety in a future release.
See https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst.
Also updates the leveldb subtree to pull: https://github.com/bitcoin-core/leveldb-subtree/pull/54.
ACKs for top commit:
davidgumberg:
Tested ACK 83bfe1485c
maflcko:
lgtm ACK 83bfe1485c37d407de7eed11b8ad769a05f78b66
theuni:
utACK 83bfe1485c37d407de7eed11b8ad769a05f78b66
Tree-SHA512: 9bc80bd04a9cebed8aca20bc23a17e52a6a89a1fb042993322f43dbf7bd93de509c091ebb69255063833b098ab11a64285eccf61e17b9f94f974c734a20ad8da
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
86e1111239cdb39dd32cfb5178653c608fa30515 test: verify node skips loading legacy wallets during startup (furszy)
9f94de5bb54ff683bd4d3a7723617b34a4706bb6 wallet: init, don't error out when loading legacy wallets (furszy)
Pull request description:
Instead of failing during initialization and shutting down the app when encountering a legacy wallet, skip loading the wallet and notify the user accordingly.
This allows users to access migration functionalities without needing to manually remove the wallet from settings.json or resort to using the bitcoin-wallet utility.
This means that GUI users will be able to use the migration button, and bitcoin-cli users will be able to call the migratewallet RPC directly after init.
ACKs for top commit:
achow101:
ACK 86e1111239cdb39dd32cfb5178653c608fa30515
w0xlt:
ACK 86e1111239
Tree-SHA512: 85d594a503ee7a833a23754b71b6ba4869ca34ed802c9ac0cd7b2fa56978f5fcad84ee4bd3acdcc61cf8e7f08f0789336febc5d76beae1eebf7bd51462512b78
Datacarrier output script sizes and output counts are now
uncapped by default.
To avoid introducing another startup argument, we modify the
OP_RETURN accounting to "budget" the spk sizes.
If a user has set a custom default, this results in that
budget being spent over the sum of all OP_RETURN outputs'
scripts in the transaction, no longer capping the number
of OP_RETURN outputs themselves. This should allow a
superset of current behavior while respecting the passed
argument in terms of total arbitrary data storage.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
ad9a13fc424e9deb262e2b1d54bcdc7370263ea0 walletdb: Log additional exception error messages for corrupted wallets (Ava Chow)
Pull request description:
Many exceptions thrown for corruption are `std::runtime_error`; we should catch those and log the message to help with debugging.
Split from #32489
ACKs for top commit:
davidgumberg:
ACK ad9a13fc42
furszy:
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
rkrux:
ACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
Sjors:
utACK ad9a13fc424e9deb262e2b1d54bcdc7370263ea0
Tree-SHA512: 107b938d67346804733ea27c44ed38822db0e020e5b1ac889ee35280d812ec56dcc9af7b3eab7a521d72cdd9cb4a8d6d35f3a3dfbcb2a6fd170a981f34fbdfc2
f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c gui: Disallow loading legacy wallets (Ava Chow)
09955172f38a4bbfa6fc8ffba3b057c73e7e3bbe wallet, rpc: Give warning in listwalletdir for legacy wallets (Ava Chow)
Pull request description:
A new field `warnings` is added for each wallet in `listwalletdir`. If a legacy wallet is detected, the warning will contain a message that the wallet is a legacy wallet and will need to be migrated before it can be loaded.
In the GUI, the "Open Wallet" menu is changed to show legacy wallets greyed out with "(needs migration)" appended to their name to indicate to the user that the legacy wallet will need to be migrated.
ACKs for top commit:
maflcko:
lgtm ACK f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c
adyshimony:
Test ACK [f3a444c](f3a444c45f)
furszy:
Code review ACK f3a444c45fb4bf4e51d53ebf1cf4c2631ded481c
w0xlt:
Code Review ACK f3a444c45f
Tree-SHA512: 496caec0ca37845487bd709e592240315eb23461fbd697e68a7fde8e4d9b74b48aab1212c88dbbcc8a107a896b824c2e1f69691068641812ae903f873fa2f22b
24e5fd3bedcebacbc10f0449be61be636b77dd79 fs: remove _POSIX_C_SOURCE defining (fanquake)
Pull request description:
On Linux systems, `_POSIX_C_SOURCE` will default to `200809L` (since glibc 2.10). There's currently no reason for us to undefine it, and then set it to an earlier value. Also tested with musl libc.
I think if anything, the project should be settings macros like `_POSIX_C_SOURCE`, globally.
ACKs for top commit:
hebasto:
re-ACK 24e5fd3bedcebacbc10f0449be61be636b77dd79, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2854183748).
Tree-SHA512: 920d60058821992193616e0c73c2f7e4230a9e3ccb9d71d16493ae69696c868f4325d3dd2d4e8388749080c187aa7b205493b3e2c6986ad37440e591ebe107e1
It reuses the logic from the `coins_view` target, except it uses an
in-memory CCoinsViewDB as the backend.
Note `CCoinsViewDB` will assert the best block hash is never null, so we
slightly modify the coins_view fuzz logic to take care of this.
83df64d7491b8271f7dfa2aea30f055102e3ff39 log: Stats when fulfilling GETBLOCKTXN (David Gumberg)
3733ed2dae3d8a6825af9f3dbc6495a238b455da log: Size of missing tx'es when reconstructing compact block (David Gumberg)
36bcee05dc71e4166be7c5a917ea9757427cece5 log: Log start of compact block initialization. (David Gumberg)
Pull request description:
This PR adds some additional logging to help measure performance of compact block reconstruction.
1. Adds a message to the beginning of `PartiallyDownloadedBlock::InitData()` so that that the logs indicate the amount of time it takes to populate a compact block from mempool transactions.
2. Logs the size of the transactions which a node did not have in its mempool and was forced to request.
3. Logs the size and number of transactions that a node sends to it's peer in a `BLOCKTXN` to fulfill a compact block `GETBLOCKTXN` request.
Relevant to this discussion on delving bitcoin: https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052
ACKs for top commit:
instagibbs:
reACK 83df64d749
w0xlt:
reACK 83df64d749
1440000bytes:
ACK 83df64d7491b8271f7dfa2aea30f055102e3ff39
Tree-SHA512: 92c3c7d55005dd47dad90ddb54e4127482260cea5390f7696e8b3b9defb337f5fb09166af6b12eb2ab8151d04dae08b0a570e3509a86509b0ab3151d84387e06
fab1e02086ceebd7d96417b7a386fe61158bfda9 refactor: Pass verification_progress into block tip notifications (MarcoFalke)
fa76b378e4b218fb4853088328b9b488de18dcd2 rpc: Round verificationprogress to exactly 1 for a recent tip (MarcoFalke)
faf6304bdfdf228354b4072b72f4c0ef90fdaade test: Use mockable time in GuessVerificationProgress (MarcoFalke)
Pull request description:
Some users really seem to care about this. While it shouldn't matter much, the diff is so trivial that it is probably worth doing.
Fixes#31127
One could also consider to split the field into two dedicated ones (https://github.com/bitcoin/bitcoin/issues/28847#issuecomment-1807115357), but this is left for a more involved follow-up and may also be controversial.
ACKs for top commit:
achow101:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
pinheadmz:
ACK fab1e02086ceebd7d96417b7a386fe61158bfda9
sipa:
utACK fab1e02086ceebd7d96417b7a386fe61158bfda9
Tree-SHA512: a3c24e3c446d38fbad9399c1e7f1ffa7904490a3a7d12623b44e583b435cc8b5f1ba83b84d29c7ffaf22028bc909c7cec07202b825480449c6419d2a190938f5
3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77 refactor: validation: mark CheckBlockIndex as const (stickies-v)
61a51eccbba1e7ccbdaac2bb7c74503bcf6fc9a5 validation: don't use GetAll() in CheckBlockIndex() (stickies-v)
d05481df644cc958cb309f20758bec996b8cfcfa refactor: validation: mark SnapshotBase as const (stickies-v)
Pull request description:
While reviewing another PR, I [noticed](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2056509235) that `ChainstateManager::CheckBlockIndex()` is not a `const` method. To try and assert that this method was not causing any side-effects, I modified the method to make it `const`. It did not surface any errors, but I think it would be good to merge this change regardless, even if `CheckBlockIndex` is only used in regtest.
This PR removes `CheckBlockIndex()`'s calls to non-const `ChainstateManager` methods by marking `SnapshotBase` `const` and ~inlining the `GetAll()` calls (thereby also performing consistency checks on invalid or fully validated `m_disabled==true` chainstates, as slight behaviour change), and finally marks `CheckBlockIndex()` as `const`.
ACKs for top commit:
achow101:
ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77
mzumsande:
Code Review ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77
TheCharlatan:
ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77
Tree-SHA512: 3d3cd351f5af1fab9a9498218ec62dba6e397fc7b5f4868ae0a77dc2b7c813d12c4f53f253f209101a3f6523695014e20c82dfac27cf0035611d5dd29feb80b5
09ee8b7f278627b917f0784adf23cbc76cae5fa0 node: avoid recomputing block hash in `ReadBlock` (Lőrinc)
2bf173210fa1ba7b28fd312b2e62bc9ff45af598 test: exercise `ReadBlock` hash‑mismatch path (Lőrinc)
Pull request description:
Eliminate one block header hash calculation per block-read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.
ACKs for top commit:
maflcko:
lgtm ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
achow101:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
jonatack:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
pinheadmz:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Tree-SHA512: 43fe51b478ea574b6d4c952684b13ca54fb8cbd67c3b6c136f460122d9ee953cc70b88778537117eecea71ccb8d88311faeac21b866e11d117f1145973204ed4
a5ac43d98d1ad3ebed934f2c50208a85aae17e5e doc: Add release notes describing bitcoin wrapper executable (Ryan Ofsky)
258bda80c009a25d1f1bdeffccf9ed1ffde29cb2 doc: Mention bitcoin wrapper executable in documentation (Ryan Ofsky)
d2739d75c911c8bf73a4d3005c57add1ae4a67ae build: add bitcoin.exe to windows installer (Sjors Provoost)
ba649c00063a43b59a63db17b509179a658a8d9a ci: Run multiprocess tests through wrapper executable (Ryan Ofsky)
29bdd743bb843f8b8ed2e426b6df36e9d7e54215 test: Support BITCOIN_CMD environment variable (Ryan Ofsky)
9c8c68891b43053acfe7b8eb9d2e0d2bcfcb4e1e multiprocess: Add bitcoin wrapper executable (Ryan Ofsky)
5076d20fdb70a4bfafc4bdfe8293e347cb6bfa78 util: Add cross-platform ExecVp and GetExePath functions (Ryan Ofsky)
Pull request description:
Intended to make bitcoin command line features more discoverable and allow installing new multiprocess binaries in libexec/ instead of bin/ so they don't cause confusion.
Idea and implementation of this were discussed in https://github.com/bitcoin/bitcoin/issues/30983.
---
Initial implementation of this feature is deliberately minimal so the UX can evolve in response to feedback and there are not too many details to debate and discuss in a single PR. But many improvements are possible or planned:
- Adding manpage and bash completions.
- Showing nicer error messages that detect if an executable isn't installed and suggest how to fix [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
- Showing wrapper command lines in subcommand in help output [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2077800405). This could be done conditionally as suggested in the comment or be unconditional.
- Showing wrapper command lines in subcommand error output. There is a bitcoin-cli error pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091152243) that is needlessly confusing.
- Integrating help so `bitcoin help subcommand` invokes `bitcoin subcommand -h`. `bitcoin -h subcommand` should also be supported and be equivalent [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
- Adding support for `bitcoin-util` subcommands. Ideal interface would probably be more like `bitcoin grind` not `bitcoin util grind` but this has been punted for now. Supporting subcommands directly would require some ArgsManager modifications
- Adding a dedicated python functional test for the wrapper. Right now there is some CI coverage by setting the `BITCOIN_CMD` variable, but this doesn't cover things like the help output and version output, and support for different directory layouts.
- Better `--multiprocess` (`-m`) / `--monolithic` (`-M`) default selection. Right now, default is monolithic but it probably makes sense to chose more intelligently depending on whether -ipc options are enabled and what binaries are available.
- Maybe parsing `bitcoin.conf` and supporting options to control wrapper behavior like custom locations or preferences or aliases.
- Better command command line usability. Allow combining short options like (`-ah`). Allow fuzzy matching of subcommands or suggestions if you misspell. (suggested by stickies in review club)
- Not directly related to this PR but `bitcoin-cli named` implementation used by the wrapper should do a better job disambiguating named arguments from base64 arguments ending in = as pointed out in [(comment)](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722). A review club meeting for it took place in https://bitcoincore.reviews/31375
ACKs for top commit:
Sjors:
utACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
achow101:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
vasild:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
theStack:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
ismaelsadeeq:
fwiw my last review implied an ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
hodlinator:
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Tree-SHA512: 570e6a4ff8bd79ef6554da3d01f36c0a7c6d2dd7dace8f8732eca98f4a8bc2284474a9beadeba783114fe2f3dd08b2041b3da7753bae0b7f881ec50668cb821f
Instead of allowing users to load a legacy wallet from the "Open Wallet"
menu, show the legacy wallet greyed out with a message that the wallet
needs to be migrated.
