05eeba2c5f [test] Add manual prune startup test case (dergoegge)
4517419628 [util] Avoid integer overflow in CheckDiskSpace (dergoegge)
Pull request description:
Starting a fresh node with `-prune=1` causes an integer overflow to happen in `CheckDiskSpace` ([here](f7bdcfc83f/src/init.cpp (L1633-L1648))) because `nPruneTarget` is to the max `uint64_t` value.
```
node1 stderr util/system.cpp:138:51: runtime error: unsigned integer overflow: 52428800 + 18446744073709551615 cannot be represented in type 'unsigned long'
#0 0x564a482b5088 in CheckDiskSpace(fs::path const&, unsigned long) src/./src/util/system.cpp:138:51
#1 0x564a4728dc59 in AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/./src/init.cpp:1639:14
#2 0x564a47256e6a in AppInit(node::NodeContext&, int, char**) src/./src/bitcoind.cpp:221:43
#3 0x564a47256087 in main src/./src/bitcoind.cpp:265:13
#4 0x7fcb7cbffd8f (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#5 0x7fcb7cbffe3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
#6 0x564a471957f4 in _start (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bitcoind+0xca07f4) (BuildId: 035cb22302d37317a630900a15a26ecb326d395c)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow util/system.cpp:138:51 in
```
I think side stepping the overflow for this specific case, is better than adding an exception to the UB suppresions file.
ACKs for top commit:
MarcoFalke:
ACK 05eeba2c5f🥝
john-moffett:
ACK 05eeba2c5f
Tree-SHA512: 1d8e6bcb49818139f04b5ab2cbef7f9b422bf0c38a804cd532b6bd0ba4c4fd07f959ba977e59896343f213086c8ecc48180f50d006638dc84649c66ec379d58a
Most of the code in util/system.cpp that was hardcoded to use the global
ArgsManager instance `gArgs` has been changed to work with explicit ArgsManager
instances (for example in https://github.com/bitcoin/bitcoin/pull/20092). But a
few hardcoded references to `gArgs` remain. This commit removes the last ones
so these functions aren't reading or writing global state.
0af16e7134 doc: add release note for #25574 (Martin Zumsande)
57ef2a4812 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb25 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cf validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134
ryanofsky:
Code review ACK 0af16e7134. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134
MarcoFalke:
lgtm re-ACK 0af16e7134 🎚
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
aadd7c5b9b refactor, validation: Add ChainstateManagerOpts db options (Ryan Ofsky)
0352258148 refactor, txdb: Use DBParams struct in CBlockTreeDB (Ryan Ofsky)
c00fa1a734 refactor, txdb: Add CoinsViewOptions struct (Ryan Ofsky)
2eaeded37f refactor, dbwrapper: Add DBParams and DBOptions structs (Ryan Ofsky)
Pull request description:
Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290#25487#25527 which remove other `ArgsManager` calls from kernel modules.
This PR does not change behavior in any way. It is a simpler alternative to #25623 because the only thing it does is remove `gArgs` references from kernel code. It avoids other unnecessary changes like adding options to the kernel API (they can be added separately later).
ACKs for top commit:
TheCharlatan:
Code review ACK aadd7c5b9b
achow101:
ACK aadd7c5b9b
furszy:
diff ACK aadd7c5b
Tree-SHA512: 46dfd5d99ab3110492e7bba97a87122c831b8344caaf7dd2ebdb6e0ad6aa9174d4d1832d6f3a7465eda9294fe50defaa3c000afbbddc4e72838687df09a63ffd
2555a3950f p2p: ProcessAddrFetch(-seednode) is unnecessary if -connect is specified (Dhruv Mehta)
Pull request description:
If the user runs: `bitcoind -connect=X -seednode=Y`, I _think_ it is safe to ignore `-seednode`. A more populated `addrman` (via `getaddr` calls to peers in `-seednode`) is not useful in this configuration: `addrman` entries are used to initiate new outbound connections when slots are open, or to open feeler connections and keep `addrman` from getting stale. This is all done in a part of `ThreadOpenConnections` (below [this line](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1803)) which is never executed when `-connect` is supplied. With `-connect`, `ThreadOpenConnections` will run [this loop](https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1785) and exit thread execution when interrupted.
Reviewers may also find it relevant that when `-connect` is used, we [soft disable](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L800) `-dnsseed` in init.cpp perhaps for the same reason i.e. seeding is not useful with `-connect`.
Running `ProcessAddrFetch` does not seem to have downside except developer confusion AFAICT. I was confused by this and felt it might affect other new bitcoiners too. If there is strong preference to not remove the line, I'd also be happy to just leave a comment there mentioning `ADDR_FETCH`/`-seednode` is irrelevant when used with `-connect`.
If this change is accepted, the node will still make `getaddr` calls to peers in `-connect` and expand `addrman`. However, disabling those `getaddr` calls would leak information about the node's configuration.
ACKs for top commit:
mzumsande:
Code Review ACK 2555a3950f
achow101:
ACK 2555a3950f
vasild:
ACK 2555a3950f
Tree-SHA512: 9187a0cff58db8edeca7e15379b1c121e7ebe8c38fb82f69e3dae8846ee94c92a329d79025e0f023c7579b2d86e7dbf756e4e30e90a72236bfcd2c00714180b3
c9d548c91f net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e9 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20 net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59a scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91f
achow101:
ACK c9d548c91f
jonatack:
re-ACK c9d548c91f only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91f
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
The rpc command verifychain now fails if the dbcache was not sufficient
to complete the verification at the specified level and depth.
In the same situation, the VerifyDB check during Init will now fail (and lead to
an early shutdown) if the user has explicitly specified -checkblocks or
-checklevel but the check couldn't be executed because of the limited
cache. If the user didn't change any of the two and is using the defaults, log a warning
but don't prevent the node from starting up.
faf7b4f1fc Add BlockManager::IsPruneMode() (MarcoFalke)
fae71fe27e Add BlockManager::GetPruneTarget() (MarcoFalke)
fa0f0436d8 Add BlockManager::LoadingBlocks() (MarcoFalke)
Pull request description:
Requested in https://github.com/bitcoin/bitcoin/pull/25781#discussion_r1061323795, but adding getters seems unrelated from removing globals, so I split it out for now.
ACKs for top commit:
dergoegge:
Code review ACK faf7b4f1fc
brunoerg:
crACK faf7b4f1fc
Tree-SHA512: 204d0e9a0e8b78175482f89b4ce620fba0e65d8e49ad845d187af44d3843f4c733a01bac1ffe5a5319f524d8346123693a456778b69d6c75268c447eb8839642
b9d5674541 init: Remove sensitive flag from rpcbind (Andrew Chow)
Pull request description:
`-rpcbind` is currently flagged as a sensitive option which means that its value will be masked when the command line args are written to the debug.log file. However this is not useful as if `rpcbind` is actually activated, the bound IP addresses will be written to the log anyways. The test `feature_config_args.py` did not catch this contradiction as the test node was not started with `rpcallowip` and so `rpcbind` was not acted upon.
This also brings `rpcbind` inline with `bind` as that is not flagged as sensitive either.
ACKs for top commit:
Sjors:
re-utACK b9d5674541
willcl-ark:
ACK b9d5674
theStack:
ACK b9d5674541
Tree-SHA512: 50ab5ad2e18ae70649deb1ac429d404b5f5c41f32a4943b2041480580152df22e72d4aae493379d0b23fcb649ab342376a82119760fbf6dfdcda659ffd3e244a
8e85164e7d doc: release note on mempool size in -blocksonly (willcl-ark)
ae797463dc doc: Update blocksonly behaviour in reduce-memory (willcl-ark)
1134686ef9 mempool: Don't share mempool with dbcache in blocksonly (willcl-ark)
Pull request description:
Fixes #9526
When `-blocksonly` has been set reduce default mempool size to avoid surprising resource usage via sharing un-used mempool cache space with dbcache.
In comparison to https://github.com/bitcoin/bitcoin/pull/9569 which either set `maxmempool` size to 0 when `-blocksonly` was set or else errored on startup, this change will permit `maxmempool` options being set.
This preserves the current (surprising?) behaviour of having a functional mempool in `-blocksonly` mode, to permit whitelisted peer transaction relay, whilst reducing average runtime memory usage for blocksonly nodes which either use the default settings or have otherwise configured a `maxmempool` size.
To use the previous old defaults node operators can configure their node with: `-blocksonly -maxmempool=300`.
ACKs for top commit:
ajtowns:
ACK 8e85164e7d
stickies-v:
re-ACK 8e85164e7d
Tree-SHA512: 1c461c24b6f14ba02cfe4e2cde60dc629e47485db5701bca3003b8df79e3aa311c0c967979f6a1dca3ba69f5b1e45fa2db6ff83352fdf2d4349d5f8d120e740d
d96d97ad30 doc: Add release note for shutdownnotify. (klementtan)
0bd73e2c45 util: Add -shutdownnotify option. (klementtan)
Pull request description:
**Description**: Similar to `-startupnotify`, this PR adds a new option to allow users to specify a command to be executed when Bitcoin Core shuts down.
**Note**: The `shutdownnotify` commands will not be executed if bitcoind shut down due to *unexpected* reasons (ie `killall -9 bitcoind`).
### Testing:
**Normal shutdown commands**
```
# start bitcoind with shutdownnotify optioin
./src/bitcoind -signet -shutdownnotify="touch foo.txt"
# shutdown bitcoind
./src/bitcoin-cli -signet stop
# check that foo.txt has been created
```
**Final RPC call**
Commands:
```
$ ./src/bitcoind -signet -nolisten -noconnect -shutdownnotify="./src/bitcoin-cli -signet getblockchaininfo > tmp.txt"
$ ./src/bitcoin-cli stop
$ cat tmp.txt
```
<details>
<summary>Screen Shot</summary>
</details>
ACKs for top commit:
achow101:
ACK d96d97ad30
1440000bytes:
ACK d96d97ad30
theStack:
re-ACK d96d97ad30
Tree-SHA512: 16f7406fd232e8b97aea5e58854c84755b0c35c88cb3ef9ee123b29a1475a376122b1e100da860cc336d4d657e6046a70e915fdb9b70c9fd097c6eef1b028161
"IP" stands for "Internet Protocol".
"IP address" is sometimes shortened to just "IP" or "address".
However, Tor or I2P addresses are not "IP addresses", nor "IPs".
Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or
I2P addresses:
`CService::ToStringIPPort()` -> `CService::ToStringAddrPort()`
`CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()`
-BEGIN VERIFY SCRIPT-
sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src)
sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src)
-END VERIFY SCRIPT-
46339d29b1 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f1 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112e p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba21 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e29 p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69c tests: stabilize sendtxrcncl test (Gleb Naumenko)
Pull request description:
Non-trivial changes include:
- Getting rid of roles in `sendtxrcncl` message (summarized in the [BIP PR](https://github.com/bitcoin/bips/pull/1376));
- Disconnect the peer if it send `sendtxrcncl` although we are in `blocksonly` and notified the peer with `fRelay=0`;
- Don't send `sendtxrcncl` to feeler connections.
ACKs for top commit:
vasild:
ACK 46339d29b1
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b1
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
7082ce3e88 scripted-diff: rename and de-globalise g_cs_orphans (Anthony Towns)
733d85f79c Move all g_cs_orphans locking to txorphanage (Anthony Towns)
a936f41a5d txorphanage: make m_peer_work_set private (Anthony Towns)
3614819864 txorphange: move orphan workset to txorphanage (Anthony Towns)
6f8e442ba6 net_processing: Localise orphan_work_set handling to ProcessOrphanTx (Anthony Towns)
0027174b39 net_processing: move ProcessOrphanTx docs to declaration (Anthony Towns)
9910ed755c net_processing: Pass a Peer& to ProcessOrphanTx (Anthony Towns)
89e2e0da0b net_processing: move extra transactions to msgproc mutex (Anthony Towns)
ff8d44d196 Remove unnecessary includes of txorphange.h (Anthony Towns)
Pull request description:
Moves extra transactions to be under the `m_msgproc_mutex` lock rather than `g_cs_orphans` and refactors orphan handling so that the lock can be internal to the `TxOrphange` class.
ACKs for top commit:
dergoegge:
Code review ACK 7082ce3e88
glozow:
ACK 7082ce3e88 via code review and some [basic testing](https://github.com/glozow/bitcoin/blob/review-26295/src/test/orphanage_tests.cpp#L150). I think putting txorphanage in charge of handling peer work sets is the right direction.
Tree-SHA512: 1ec454c3a69ebd45ff652770d6a55c6b183db71aba4d12639ed70f525f0035e069a81d06e9b65b66e87929c607080a1c5e5dcd2ca91eaa2cf202dc6c02aa6818
6630a1e844 Add warning on first startup if free disk space is less than necessary (Ben Woosley)
Pull request description:
This reworks/revives https://github.com/bitcoin/bitcoin/pull/15848 to add a check for low disk space on first startup and issue a warning if disk space is below the expected space required to accommodate the blocks.
This PR was fashioned by a team of developers at the [bitcoin++](https://www.btcplusplus.dev/) conference workshop: "[Let's contribute to Bitcoin Core](https://sched.co/12P6Z)"
Fixes #15813
ACKs for top commit:
achow101:
ACK 6630a1e844
willcl-ark:
tACK 6630a1e844 rebased on master. Warning shows on first start but not on restart after some blocks have been downloaded.
aureleoules:
ACK 6630a1e844
pablomartin4btc:
re-ACK 6630a1e844
hernanmarino:
ReACK 6630a1e844
Tree-SHA512: 0f18acabdf2b514e96e2eea8f304960b952226b83dc91334cf7d1f6355ea2f257aaec0ee38d43ac36435385ecd918333d20657c35a8a7407e7cf2680ccb643bb
aaaa7bd0ba iwyu: Add missing includes (MacroFake)
fa9ebec096 Remove g_parallel_script_checks (MacroFake)
fa7c834b9f Move ::fCheckBlockIndex into ChainstateManager (MacroFake)
fa43188d86 Move ::fCheckpointsEnabled into ChainstateManager (MacroFake)
cccca83099 Move ::nMinimumChainWork into ChainstateManager (MacroFake)
fa29d0b57c Move ::hashAssumeValid into ChainstateManager (MacroFake)
faf44876db Move ::nMaxTipAge into ChainstateManager (MacroFake)
Pull request description:
It seems preferable to assign globals to a class (in this case `ChainstateManager`), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.
ACKs for top commit:
dergoegge:
Code review ACK aaaa7bd0ba
ryanofsky:
Code review ACK aaaa7bd0ba. No changes since last review, other than rebase
aureleoules:
reACK aaaa7bd0ba
Tree-SHA512: 83ec3ba0fb4f1dad95810d4bd4e578454e0718dc1bdd3a794cc4e48aa819b6f5dad4ac4edab3719bdfd5f89cbe23c2740a50fd56c1ff81c99e521c5f6d4e898d
b147322a7a Use `PACKAGE_NAME` in messages rather than hardcoding "Bitcoin Core" (Hennadii Stepanov)
Pull request description:
Usually, we do not hardcode "Bitcoin Core" in the user-faced messages.
See:
- bitcoin/bitcoin#18646
- bitcoin/bitcoin#19282
Also grammar has been improved -- singular instead of plural.
ACKs for top commit:
jarolrod:
ACK b147322a7a
Tree-SHA512: b135c18703dfdd7b63d4cb27d1ac48f6a9dbf69382142ae381f33bf561cbf57477a11d1c73263aa834f705206d7dd5716df2523d38ed0d4cfec8babc38bb017a
This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).
This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.
If we're connecting to the peer which might support
transaction reconciliation, we announce we want to reconcile
with them.
We store the reconciliation salt so that when the peer
responds with their salt, we are able to compute the
full reconciliation salt.
This behavior is enabled with a CLI flag.
Check `port` options for invalid values (ports are parsed as uint16, so
in practice values >65535 are invalid; port 0 is undefined and therefore
considered invalid too). This allows for an early rejection of faulty
values and an supplying an informative message to the user.
Splits tests in `feature_proxy.py` to cover both invalid `hostname`
and `port` values.
Adds a release-note as previously valid `-port` and `-rpcport` values
can now result in errors.
68209a7b5c rpc: make addpeeraddress work with cjdns addresses (Martin Zumsande)
a8a9ed67cc init: Abort if i2p/cjdns are chosen via -onlynet but unreachable (Martin Zumsande)
Pull request description:
If the networks i2p / cjdns are chosen via `-onlynet` but the user forgot to provide `-i2psam` / `-cjdnsreachable`, no outbound connections will be made - it would be nice to inform the user about that.
The solution proposed here mimics existing behavior for `-onlynet=onion` and non-specified `-onion`/`-proxy` where we already abort with an InitError - if reviewers would prefer to just print a warning, please say so.
The second commit adds CJDNS support to the debug-only `addpeeraddress` RPC allowing to add CJDNS addresses to addrman for testing and debug purposes. (if `-cjdnsreachable=1`)
This is the result of an [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-09-01#848066;) with vasild.
ACKs for top commit:
vasild:
ACK 68209a7b5c
dergoegge:
ACK 68209a7b5c
Tree-SHA512: 6db9787f01820190f14f90a0b39e4206603421eb7521f792879094d8bbf4d4d0bfd70665eadcc40994ac7941a15ab5a8d65c4779fba5634c0e6fa66eb0972b8d
fa521c9603 Use steady clock for all millis bench logging (MacroFake)
Pull request description:
Currently `GetTimeMillis` is used for bench logging in milliseconds integral precision. Replace it to use a steady clock that is type-safe and steady.
Microsecond or float precision can be done in a follow-up.
ACKs for top commit:
fanquake:
ACK fa521c9603 - started making the same change.
Tree-SHA512: 86a810e496fc663f815acb8771a6c770331593715cde85370226685bc50c13e8e987e3c5efd0b4e48b36ebd2372255357b709204bac750d41e94a9f7d9897fa6
