956c67059c refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande)
3c43d9db1e p2p: Don't self-advertise during VERSION processing (Gleb Naumenko)
Pull request description:
This picks up the last commit from #19843.
Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer.
This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after
the version handshake is finished.
There are a couple of differences:
1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones.
2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable.
3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`.
Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`.
ACKs for top commit:
stratospher:
ACK 956c670
naumenkogs:
ACK 956c67059c
amitiuttarwar:
reACK 956c67059c
Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
8f5c560e11 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b9491b refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)
Pull request description:
Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570)
ACKs for top commit:
stickies-v:
re-ACK [8f5c560](8f5c560e11)
Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
293849a260 univalue: Remove confusing getBool method (Ryan Ofsky)
Pull request description:
Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.
The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate.
These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR.
ACKs for top commit:
justinpickering:
ACK 293849a260
sipa:
utACK 293849a260
w0xlt:
ACK 293849a260
hebasto:
ACK 293849a260, also verified that the removed `getBool` method is not mentioned in any docs:
furszy:
ACK 293849a2
Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
We need to check that the fee is not negative even before it is
finalized. The setting of fees for SFFO may adjust the fee to be
"correct" and no longer negative, but erroneously reduce the amounts too
far. So we need to check this condition before we do those adjustments.
It doesn't make sense to be checking whether the fee paid is underpaying
before we've finished setting the fees. So do that after we have done
the reduction for SFFO and change adjustment for fee overpayment.
The logest running tests should be at the front of the list in
test_runner.py. Since compiling with --enable-debug can have a
significant effect on test runtime, the order is based on the runtime
with that option configured.
test_submit_child_with_parents creates a p2p connection which waits for
the node to announce transactions to it. By whitelisting this
connection, we can reduce the amount of time spent waiting for this
announcement which improves the test runtime and runtime variance.
The work queue exceeded test in interface_rpc.py would repeatedly call
an RPC until the error was achieved. However hitting this error is
dependent on the processing speed of the computer and the optimization
level of the binary. Configurations that result in slower processing
would result in the RPC used being processed before the error could be
hit, resulting the test's runtime having a high variance.
Switching the RPC to waitfornewblock allows it to run in a much more
consistent time that is still fairly fast. waitfornewblock forces
the RPC server to allocate a thread and wait, occupying a spot in the
work queue. This is perfect for this test because the slower the RPC,
the more likely we will achieve the race condition necessary to pass the
test. Using a timeout of 500 ms appears to work reliably without causing
the test to take too long.
The sigops draining script in feature_taproot's block_submit was
initialized with a list that would end up always being iterated by
CScript's constructor. Since this list is very large, a lot of time
would be wasted. By creating and passing a bytes object initialized from
that list, we can avoid this iteration and dramatically improve the
runtime of feature_taproot.
feature_fee_estimation has a lot of loops that hit the RPC many times in
succession in order to setup scenarios. Using batched requests for these
can reduce the test's runtime without effecting the test's behavior.
Generating blocks is slow, especially when --enable-debug. There is no
need to generate a new block for each transaction, so avoid doing that
to improve this test's runtime.
fa7d71accc test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py (MarcoFalke)
fa933d6985 test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py (MarcoFalke)
Pull request description:
The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that.
ACKs for top commit:
fanquake:
ACK fa7d71accc
pablomartin4btc:
re-ACK fa7d71accc
Tree-SHA512: 9dc131ed8ff119bd6d43d04ecc5c96d02f2d6fdc3d0805492774a414c0fbe6a984da7631154122a080c54ddd25fc5a5bdd52b282c918fce4932057ba72c2bf71
Replacing `install` with `install-lib` and `install-bin` is not strictly
necessary just to update the library, but it takes advantage of recent
changes in the new version, and makes the build more minimal.
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.
The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.
These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
fabb24cbef test: Use last release in compatibility tests (MarcoFalke)
Pull request description:
In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release.
ACKs for top commit:
Sjors:
utACK fabb24c
Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
affbf58a1e build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3c97 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.
Another [bug](https://github.com/bitcoin/bitcoin/issues/22719) in the depends build system has already become a problem at least two times in the past (https://github.com/bitcoin/bitcoin/pull/16883#issuecomment-683817472 and https://github.com/bitcoin/bitcoin/pull/24134). Each time the problem was solved with other means.
The initial [solution](https://github.com/bitcoin/bitcoin/pull/19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](https://github.com/bitcoin/bitcoin/pull/24134).
The bug is well described in bitcoin/bitcoin#22719.
Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
After creating targets by this code:1e7564eca8/depends/funcs.mk (L280) a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).
Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
```sh
$ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
$ echo $?
1
```
Using the `export` command is a trivial solution:
```sh
$ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
foo
bar
end
$ echo $?
0
```
As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.
Fixesbitcoin/bitcoin#22719.
---
Also this PR removes no longer needed crutch from `qt.mk`.
ACKs for top commit:
fanquake:
ACK affbf58a1e
Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
fa579f3063 refactor: Pass reference to last header, not pointer (MacroFake)
Pull request description:
It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
ACKs for top commit:
john-moffett:
ACK fa579f3
aureleoules:
ACK fa579f3063
Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
07dfbb5bb8 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès)
Pull request description:
Fixes#22189.
The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables.
ACKs for top commit:
jamesob:
ACK 07dfbb5bb8 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a))
theStack:
Concept and code-review ACK 07dfbb5bb8
Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
d7f61e7d59 rpc: reduce LOCK(cs_main) scope in gettxoutproof (Andrew Toth)
4d92b5aaba rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstats (Andrew Toth)
efd82aec8a rpc: reduce LOCK(cs_main) scope in blockToJSON (Andrew Toth)
f00808e932 rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblock (Andrew Toth)
7d253c943f zmq: remove LOCK(cs_main) from NotifyBlock (Andrew Toth)
c75e3d2772 rest: reduce LOCK(cs_main) scope in rest_block (Andrew Toth)
Pull request description:
Picking up from #21006.
After commit ccd8ef65f9 it is no longer required to hold `cs_main` when calling `ReadBlockFromDisk`. This can be verified in `master` at https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L755. Same can be seen for `UndoReadFromDisk` https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L485.
The first commit moves `ReadBlockFromDisk` outside the lock scope in `rest_block`, where we can see a huge performance improvement when fetching blocks with multiple threads.
My test setup, on an Intel i7 with 8 cores (16 threads):
1. Start a fully synced bitcoind, with this `bitcoin.conf`:
```
rest=1
rpcthreads=16
rpcworkqueue=64
rpcuser=user
rpcpassword=password
```
2. Run ApacheBench: 10000 requests, 16 parallel threads, fetching block nr. 750000 in binary:
```
ab -n 10000 -c 16 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"
```
Time per request (mean)
183 ms on master
30 ms this branch
So this can process 6.1 times as many requests, and saturates all the cores instead of keeping them partly idle waiting in the lock. With 8 threads the mean times were 90 ms on master and 19 ms on this branch, a speedup of 4.7x.
Big thanks to martinus for finding this and the original PR.
The second commit is from a suggestion on the original PR by jonatack to remove the unnecessary `LOCK(cs_main)` in the zmq notifier's `NotifyBlock`.
I also found that this approach could be applied to rpcs `getblock` (including `verbosity=3`), `getblockstats`, and `gettxoutproof` with similar very good results. The above benchmarks steps need to be modified slightly for RPC. Run the following ApacheBench command with different request data in a file named `data.json`:
```
ab -p data.json -n 10000 -c 16 -A user:password "http://127.0.0.1:8332/"
```
For `getblock`, use the following in `data.json`:
```
{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e"]}
```
master - 184 ms mean request time
branch - 28 ms mean request time
For `getblock` with verbosity level 3, use the following in `data.json`:
```
{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 3]}
```
This verbosity level fetches an undo file from disk, so it benefits from this approach as well. However, a lot of time is spent serializing to JSON so the performance gain is not as severe.
master - 818 ms mean request time
branch - 505 ms mean request time
For `getblockstats`, use the following in `data.json`:
```
{"jsonrpc": "1.0", "id": "curltest", "method": "getblockstats", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", ["minfeerate","avgfeerate"]]}
```
This request used a lock on reading both a block and undo file, so the results are very good.
master - 244 ms mean request time
branch - 28 ms mean request time
ACKs for top commit:
MarcoFalke:
re-ACK d7f61e7d59💫
hebasto:
ACK d7f61e7d59, I have reviewed the code and it looks OK. Did not make benchmarking though.
Tree-SHA512: 305ac945b4571c5f47646d4f0e78180d7a3d40b2f70ee43e4b3e00c96a465f6d0b9c750b8e85c89ed833e557e2cdb5896743f07ef90e4e53d4ad85452b545886
4e362c2b72 doc: add release note for 25934 (brunoerg)
fe488b4c4b test: add coverage for `label` in `listsinceblock` (brunoerg)
722e9a418d wallet, rpc: add `label` to `listsinceblock` (brunoerg)
852891ff98 refactor, wallet: use optional for `label` in `ListTransactions` (brunoerg)
Pull request description:
This PR adds `label` parameter to `listsinceblock` to be able to fetch all incoming transactions having the specified label since a specific block.
It's possible to use it in `listtransactions`, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. `getreceivedbylabel` only returns the total amount received, not the txs info. `listreceivedbylabel` doesn't list all the informations about the transactions and it's not possible to fetch since a block.
ACKs for top commit:
achow101:
ACK 4e362c2b72
w0xlt:
ACK 4e362c2b72
aureleoules:
ACK 4e362c2b72
Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737
ccba4fe7e3 doc: Add completion subdir to contrib/README.md (willcl-ark)
7075848f96 script: Add fish completions (willcl-ark)
a27a445b71 refactor: Sub-folder bash completions (willcl-ark)
Pull request description:
The completions are dynamically generated from the respective binary
help pages.
Completions should be sourced into the shell or added to
`$XDG_CONFIG/fish/completions`. See [where to put completions](https://fishshell.com/docs/current/completions.html#where-to-put-completions) for more information.
As the completions are auto-generated they should only require as much maintenance as the bash equivalents, which is to say very little!
ACKs for top commit:
achow101:
ACK ccba4fe7e3
josibake:
ACK ccba4fe7e3
Tree-SHA512: fe6ed899ea1fe90f82970bde7739db11dd0c845ccd70b65f28ad5212f75b57d9105a3a7f70ccdff552d5b21fa3fe9c697d128fb10740bae31fe1854e716b4b8b
This makes the code more robust, see previous commit.
In general replacing isTrue with get_bool is not equivalent because
get_bool can throw exceptions, but in this case, exceptions won't happen
because of RPCTypeCheck() and isNull() checks in the preceding code.
6fb102c9f3 test: Changed small_txpuzzle_randfee to return the virtual size instead of the transaction hex for feerate calculation. (Randall Naar)
Pull request description:
The fee rates used in feature_fee_estimation.py are calculated using the raw transaction size instead of the virtual transaction size (which is used in 'CBlockPolicyEstimator::processBlockTx' and 'CBlockPolicyEstimator::processBlock'). This leads to inconsistencies as the fee rates used in check_raw_estimates are incorrect and can cause assertions to fail.
refs #25179
ACKs for top commit:
MarcoFalke:
ACK 6fb102c9f3
Tree-SHA512: b2bca85fffa605aeb17574f050736d4556506d782dd7fd969e165e48e108fd95ef4c4e2abbae83cce05ca777a00f4459cabfa0932694fa8bb93ddfba09d84357
b19c4124b3 refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky)
dd6e8bd71c build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake)
82e272a109 refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a (Ryan Ofsky)
Pull request description:
These belong in `libbitcoin_common.a`, not `libbitcoin_util.a`, because they aren't general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in `libbitcoin_util.a` is to prevent them from being used by the kernel library.
Also rename ambiguous `MakeHandler` functions to `MakeCleanupHandler` and `MakeSignalHandler`. Cleanup function handler was introduced after boost signals handler, so original naming didn't make much sense.
This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes.
This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the _util_ library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase.
ACKs for top commit:
hebasto:
ACK b19c4124b3
Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
fa825bd227 util: Include full version id in bug reports (MarcoFalke)
Pull request description:
This will show the unique id of the full source code when the bug occurred, which can help debugging
ACKs for top commit:
1440000bytes:
utACK fa825bd227
theStack:
ACK fa825bd227
john-moffett:
ACK fa825bd227
Tree-SHA512: a7a775718f5f9796b5cffafbb3ace8adb5c163414ec584a57143157fc9dfb86f799e3b9c8365fcb831ee1e9eafc59d699d1653d772c68392de421b3de74dcd61
