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
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
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
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>
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
faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40 doc: Remove ParseInt mentions in documentation (MarcoFalke)
33332829333b589420f8038541d04ec6970f051d refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c36cb0accf87123ca4eb98f6219d55fb5e bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face2519fac9e840d52f0334d9079e664be7eb28 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf0b993c879ee8c516baa36339ff7b72406 bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a558ab15b5d8eeea5dd4c9c0d76350051b bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5fe38b81f1af6b343661b65e16b0de7c60 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac03788243847b799a3feaac56bc918fba9 rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb499dec79258b1857b404d72f93650503f4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c69192fcb48a9e04d52eeb07fff15655d0 test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123afa0ef752e8645bf695d121da66d8f1382b Reject + sign when checking -ipcfd (MarcoFalke)
fa479857ed234d54df31d33b60de14c6ffab3d6f Reject + sign in SplitHostPort (MarcoFalke)
fab4c2967d554ddbc15f732cea6cd190c547d89f net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652e68fc07fb6c9f3d9e34dc11d35f0cc1e1 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c45577dfbae67535e4965b5660288557d3631 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa980413257e2004a8d48a8be66c6d67f90b76ad refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7fc24212d85cdc7f52b317906b37a1a120 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)
Pull request description:
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:
* Useless, because the `+` sign was already rejected.
* Erroneous and inconsistent, when third party parsers reject it. (C.f. https://github.com/bitcoin/bitcoin/pull/32365)
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.
Fix all issues by removing the legacy int parsing.
ACKs for top commit:
stickies-v:
re-ACK faf55fc80b
brunoerg:
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
We need to determine if CJDNS is reachable before we parse any IPv6
addresses (for example, by the -rpcallowip setting) or an RFC4193
address might get flipped to CJDNS, which can not be used with subnets
`-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
It is confusing that the chain client flush happens between
StopHTTPServer and StopMapPort. Also, it is unused code. Seems best to
just add it back properly when it is needed again.
Rename the `_randomize_credentials` parameter to Proxy's constructor to
`tor_stream_isolation` to make it more clear, and more specific what its
purpose is.
Also change all call sites to use a named parameter.
Checking for IsArgSet before calling GetArg while providing an arbitrary
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
Even better would be to provide the true fallback value and sanitize it
as if it were user-input, but this can be done in a follow-up.
Removing the redundant call to IsArgSet will have to be done either way,
so do it now.
Checking for IsArgSet before calling GetArg while providing the args
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
However, ignoring the fallback value is fragile, because it would not be
sanitized.
Fix all issues by sanitizing the fallback value.
Closesbitcoin-core/gui#843.
In that issue it was brought up that users likely don't care what kind
of port forwarding is used, and the setting is opportunistic anyway, so
instead of showing an extensive warning, we can simply migrate from
UPNP to NAT-PMP+PCP. This prevents nodes dropping from the public
network.
- Change the logic for removed runtime setting `-upnp` to set `-natpmp`
instead, and only log a message.
- Also replace any lingering `upnp` in `settings.json` with `natpmp`.
This commit does not change behavior, it just changes code to handle -noconnect
values explicitly with IsArgNegated() instead of implicitly with IsArgSet(),
and adds comments to make it clear what behavior is intended when -noconnect is
specified.
Treat specifying -noexternalip the same as not specifying -externalip, instead
of causing it to soft-set the -discover default to false.
Before this change, was -noexternalip basically an undocumented synonym for
-nodiscover.
After this change, specifying -noexternalip just clears previously specifed
-externalip options, restoring default behavior as if they were not were
specified.
The previous -noexternalip behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -nodiscover option, and inconsistent with
behavior of other list options.
Treat specifying -noonlynet the same as not specifying -onlynet, instead of
marking all networks unreachable.
Before this change, specifying -noonlynet cleared list of reachable networks
and did not allow connecting to any network. It was basically an undocumented
synonym for -noconnect.
After this change, specifying -nononlynet just clears previously specifed
-onlynet options and allows connecting to all networks, restoring default
behavior as if no -onlynet options were specified.
Before this change, there was no way to restore default behavior once an
-onlynet option was specified. So for example, if a config file specifed
onlynet settings, they couldn't be reset on the command line without disabling
the entire config file.
The previous -noonlynet behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -noconnect option, inconsistent with behavior
of other list options, and inconsistent with being able to use the command line
to selectively override config options. It was also probably unintended,
arising from use of the IsArgSet() method and its interaction with negated
options.
386eecff5f14d508688e6e7374b67cb54aaa7249 doc: add release notes (ismaelsadeeq)
3eaa0a3b663782bb1bd874ea881b21649f1db767 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd14841e35ce39d7a6f51131e6a41de2 doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d5a7617764857b51777c076fd7ef13d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc9155ed58ad97fc04b47b3d317a3ec2 test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d67a159332d109aab278632d64078f0b miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)
Pull request description:
* This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
* Fixes#21950
We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`
7590e93bc7/src/policy/policy.h (L23)
And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.
7590e93bc7/src/node/types.h (L36-L40)
**Motivation**
- This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
- It was later reported in issue #21950.
- I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.
---
This PR fixes this by consolidating the reservation to be in a single location in the codebase.
This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.
ACKs for top commit:
Sjors:
ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
fjahr:
Code review ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
glozow:
utACK 386eecff5f14d508688e6e7374b67cb54aaa7249, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f14d508688e6e7374b67cb54aaa7249
Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c
- This commit renamed coinbase_max_additional_weight to block_reserved_weight.
- Also clarify that the reservation is for block header, transaction count
and coinbase transaction.
0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d kernel: Move block tree db open to BlockManager constructor (TheCharlatan)
7fbb1bc44b1461f008284533f1667677e729f0c0 kernel: Move block tree db open to block manager (TheCharlatan)
57ba59c0cdf20de322afabe4a132ad17e483ce77 refactor: Remove redundant reindex check (TheCharlatan)
Pull request description:
Before this change the block tree db was needlessly re-opened during startup when loading a completed snapshot. Improve this by letting the block manager open it on construction. This also simplifies the test code a bit.
The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
ACKs for top commit:
maflcko:
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d 🏪
achow101:
ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
mzumsande:
re-ACK 0cdddeb2240d1f33c8b2dd28bb0c9d84d9420e3d
Tree-SHA512: fe3d557a725367e549e6a0659f64259cfef6aaa565ec867d9a177be0143ff18a2c4a20dd57e35e15f97cf870df476d88c05b03b6a7d9e8d51c568d9eda8947ef
2656a5658c14b43c32959db7235e9db55a17d4c8 tests: add a test for the new blocksdir lock (Cory Fields)
bdc0a68e676f237bcbb5195a60bb08bb34123e71 init: lock blocksdir in addition to datadir (Cory Fields)
cabb2e5c24282c88ccc7fcaede854eaa8d7ff162 refactor: introduce a more general LockDirectories for init (Cory Fields)
1db331ba764d27537d82abd91dde50fc9d7e0ff4 init: allow a new xor key to be written if the blocksdir is newly created (Cory Fields)
Pull request description:
This probably should've been included in #12653 when `-blocksdir` was introduced. Credit TheCharlatan for noticing that it's missing.
This guards against 2 processes running with separate datadirs but the same blocksdir. I didn't add `walletdir` as I assume sqlite has us covered there.
It's not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.
ACKs for top commit:
maflcko:
review ACK 2656a5658c14b43c32959db7235e9db55a17d4c8 🏼
kevkevinpal:
ACK [2656a56](2656a5658c)
achow101:
ACK 2656a5658c14b43c32959db7235e9db55a17d4c8
tdb3:
Code review and light test ACK 2656a5658c14b43c32959db7235e9db55a17d4c8
Tree-SHA512: 3ba17dc670126adda104148e14d1322ea4f67d671c84aaa9c08c760ef778ca1936832c0dc843cd6367e09939f64c6f0a682b0fa23a5967e821b899dff1fff961
223081ece651dc616ff63d9ac447eedc5c2a28fa scripted-diff: rename block and undo functions for consistency (Lőrinc)
baaa3b284671ba28dbbcbb43851ea46175fd2b13 refactor,blocks: remove costly asserts and modernize affected logs (Lőrinc)
fa39f27a0f8b8d14f6769d48f43999a3a1148e4f refactor,blocks: deduplicate block's serialized size calculations (Lőrinc)
dfb2f9d004860c95fc6f0d4a016a9c038d53a475 refactor,blocks: inline `WriteBlockToDisk` (Lőrinc)
42bc4914658d9834a653bd1763aa8f0d54355480 refactor,blocks: inline `UndoWriteToDisk` (Lőrinc)
86b85bb11f8999eb59e34bd026b0791dc866f2eb bench: add SaveBlockBench (Lőrinc)
34f9a0157aad7c10ac364b7e4602c5f74c1f9e20 refactor,bench: rename bench/readblock.cpp to bench/readwriteblock.cpp (Lőrinc)
Pull request description:
`UndoWriteToDisk` and `WriteBlockToDisk` were delegating a subset of their functionality to single-use methods that didn't optimally capture a meaningful chunk of the algorithm, resulting in calculating things twice (serialized size, header size).
This change inlines the awkward methods (asserting that all previous behavior was retained), and in separate commits makes the usages less confusing.
Besides making the methods slightly more intuitive, the refactorings reduce duplicate calculations as well.
The speed difference is insignificant for now (~0.5% for the new `SaveBlockToDiskBench`), but are a cleanup for follow-ups such as https://github.com/bitcoin/bitcoin/pull/31539
ACKs for top commit:
ryanofsky:
Code review ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.
hodlinator:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
TheCharlatan:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
andrewtoth:
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa
Tree-SHA512: 951bc8ad3504c510988afd95c561e3e259c6212bd14f6536fe56e8eb5bf5c35c32a368bbdb1d5aea1acc473d7e5bd9cdcde02008a148b05af1f955e413062d5c
Make the block db open RAII style by calling it in the BlockManager
constructor.
Before this change the block tree db was needlessly re-opened during
startup when loading a completed snapshot. Improve this by letting the
block manager open it on construction. This also simplifies the test
code a bit.
The change was initially motivated to make it easier for users of the
kernel library to instantiate a BlockManager that may be used to read
data from disk without loading the block index into a cache.
This commit is done in preparation for the next commit. Here, the block
tree options are moved to the blockmanager options and the block tree is
instantiated through a helper method of the BlockManager, which is
removed again in the next commit.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
This guards against 2 processes running with separate datadirs but the same
blocksdir.
It's not likely to happen currently, but may be more relevant in the future
with applications using the kernel.
Note that the kernel does not currently do any dir locking, but it should.
2a92702bafca5c78b270a9502a22cb9deac02cfc init: Use size_t consistently for cache sizes (TheCharlatan)
65cde3621dbb9ac7d210d4926e7601c4adf5f498 kernel: Move default cache constants to caches (TheCharlatan)
8826cae285490439dc1f19b25fa70b2b9e62dfe8 kernel: Move non-kernel db cache size constants (TheCharlatan)
e758b26b85da27ef44f3d2c924f3f08e8c1f4fdf kernel: Move kernel-specific cache size options to kernel (TheCharlatan)
d5e2c4a4097c799433cfc5367c61568fad2c784e fuzz: Add fuzz test for checked and saturating add and left shift (TheCharlatan)
c03a2795a8e044d17835bbf03de0c64dc7b41da8 util: Add integer left shift helpers (TheCharlatan)
8bd5f8a38ce903c05606841ebed1902398cb0b14 [refactor] init: Simplify coinsdb cache calculation (TheCharlatan)
5db7d4d3d28bd1269a09955b4695135c86c4827d doc: Correct docstring describing max block tree db cache (TheCharlatan)
Pull request description:
Carrying non-kernel related fields in the cache sizes for the indexes is confusing for kernel library users. The cache sizes are set currently with magic numbers in bitcoin-chainstate. The comments for the cache size calculations are not completely clear. The constants for the cache sizes are also currently in `txdb.h`, which is not an ideal place for holding all cache size related constants.
Solve these things by moving the kernel-specific cache size fields to their own struct and moving the constants to either the node or the kernel cache sizes.
This slightly changes the way the cache is allocated if (and only if) the txindex and/or blockfilterindex is used. Since they are now given precedence over the block tree db cache, this results in a bit less cache being allocated to the block tree db, coinsdb and coins caches. The effect is negligible though, i.e. cache sizes with default dbcache reported through the logs are:
master:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
this PR:
```
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
```
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
stickies-v:
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc
ryanofsky:
Code review ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc. Changes since last review are fixing size options to use size_t instead of int64_t again, simplifying CheckedLeftShift more, and making other minor suggested cleanups
hodlinator:
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc
Tree-SHA512: 98376eaa0660b1b8c096a5ce1f3e7c8c30e7cd6644de36856c2d3e573108cfc9473c93ebb3952b7881047b5ae6c85c5b096e6726f30f35be58b98eca07c8c785
This avoids having to rely on implicit casts when passing them to the
various functions allocating the caches.
This also ensures that if the requested amount of db_cache does not fit
in a size_t, it is clamped to the maximum value of a size_t.
Also take this opportunity to make the total amounts of cache in the
chainstate manager a size_t too.
They are not related to the txdb, so a better place for them is the
new kernel and node cache file. Re-use the default amount of kernel
cache for the default node cache.
Carrying non-kernel related fields in the cache sizes for the indexes is
confusing for kernel library users. The cache sizes also are set
currently with magic numbers in bitcoin-chainstate. The comments for the
cache size calculations are also not completely clear.
Solve these things by moving the kernel-specific cache size fields to
their own struct.
This slightly changes the way the cache is allocated if the txindex
and/or blockfilterindex is used. Since they are now given precedence
over the block tree db cache, this results in a bit less cache being
allocated to the block tree db, coinsdb and coins caches. The effect is
negligible though, i.e. cache sizes with default dbcache reported
through the logs are:
master:
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.0 MiB for transaction index database
* Using 49.0 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 335.0 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
this branch:
Cache configuration:
* Using 2.0 MiB for block index database
* Using 56.2 MiB for transaction index database
* Using 49.2 MiB for basic block filter index database
* Using 8.0 MiB for chain state database
* Using 334.5 MiB for in-memory UTXO set (plus up to 286.1 MiB of unused mempool space)
Example logs before the change:
```
2025-01-07T11:58:33Z Verification progress: 99%
2025-01-07T11:58:33Z Verification: No coin database inconsistencies in last 6 blocks (18905 transactions)
2025-01-07T11:58:33Z block index 31892ms
2025-01-07T11:58:33Z Setting NODE_NETWORK on non-prune mode
2025-01-07T11:58:33Z block tree size = 878086
2025-01-07T11:58:33Z nBestHeight = 878085
```
Removed redundant duration as well since it can be recovered from the timestamps.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
e56fc7ce6a92eae7e80657d9f57a148cc002358d rpc: increase the defaults for -rpcthreads and -rpcworkqueue (Vasil Dimov)
Pull request description:
`rpcthreads` was introduced with a default of 4 in 2013 in 21eb5adadbe3110a8708f2570185566e1f137a49
`rpcworkqueue` was introduced with a default of 16 in 2015 in 40b556d3742a1f65d67e2d4c760d0b13fe8be5b7
Resolves: https://github.com/bitcoin/bitcoin/issues/29386
---
Just bump the ancient default values. There is no perfect default that would fit everybody. This could lead to https://bikeshed.com/
ACKs for top commit:
achow101:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
andrewtoth:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
storopoli:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
tdb3:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
Tree-SHA512: ba3ea7392fda57950daa6b4c4d38ecdef9eebe5e786824d25f8b5cea03e760ffff7f77f3acd8eb6c6178b1e92b282e02cabb940ed7222eec7f73efdb819eef06
81cea5d4ee0e226f7c7145072de85829f4166ec9 Ensure m_tip_block is never ZERO (Sjors Provoost)
e058544d0e83aed691903d13d7e5775beb7e678f Make m_tip_block an std::optional (Sjors Provoost)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309
ACKs for top commit:
fjahr:
re-ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9
tdb3:
code review re ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9
l0rinc:
ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9
Tree-SHA512: 31a75ba29e3d567bab32e4e7925a419d9d7a4d2d85ed1c1012116d8d22adc14d31d5b4ce5f6c499c994188dcd26a01cced05be74f94c892fc90ae17a6783a472
facb4d010ca5c898756bedee46069509900ebea0 refactor: Move GuessVerificationProgress into ChainstateManager (MarcoFalke)
Pull request description:
Currently the function is standalone, which means any passed-in data like `TxData` or the block pointer needs to be taken from the `ChainstateManager` and passed in. This is currently verbose and may become even more verbose if the function is reworked in the future. As the function can not be called without a `ChainstateManager` in production code anyway, make it a member function on the class.
ACKs for top commit:
ryanofsky:
Code review ACK facb4d010ca5c898756bedee46069509900ebea0. Nice cleanup, that should make this code less awkward to work with
TheCharlatan:
ACK facb4d010ca5c898756bedee46069509900ebea0
danielabrozzoni:
reACK facb4d010ca5c898756bedee46069509900ebea0
Tree-SHA512: b17977e12cd7c6e308c47e6a1aa920acecd4442696e46d1f30bd7c201e9898ca2d581ff0bf2cc9f7334e146c1b0c50925adb849c8c17f65dcdf6877be1c5f776
To avoid future code changes from reintroducing the ambiguity fixed
by the previous commit, mark m_tip_block private and Assume that
it's not set to uint256::ZERO.
