| Age | Commit message (Collapse) | Author |
|---|
|
pointer
fa579f306363ed03c1138121415b67b9b36b4d53 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 fa579f306363ed03c1138121415b67b9b36b4d53
Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
|
|
Chainstate members
07dfbb5bb8115c680621f361c65d9cde2f8c52f2 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 07dfbb5bb8115c680621f361c65d9cde2f8c52f2 ([`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 07dfbb5bb8115c680621f361c65d9cde2f8c52f2
Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
|
|
times as many requests per second
d7f61e7d5909da7227c9e34be06ce9eb872ba074 rpc: reduce LOCK(cs_main) scope in gettxoutproof (Andrew Toth)
4d92b5aabaf371225cdc37a7b114adc040bf4da5 rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstats (Andrew Toth)
efd82aec8a2dd0fca8f2597c3f84cefe057d1243 rpc: reduce LOCK(cs_main) scope in blockToJSON (Andrew Toth)
f00808e932c1f67bc76702a3f05778074b10025c rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblock (Andrew Toth)
7d253c943f44612431be894b198ffb49ff988fff zmq: remove LOCK(cs_main) from NotifyBlock (Andrew Toth)
c75e3d2772b00acc3850f72a8cb733a0345aa773 rest: reduce LOCK(cs_main) scope in rest_block (Andrew Toth)
Pull request description:
Picking up from #21006.
After commit https://github.com/bitcoin/bitcoin/commit/ccd8ef65f93ed82a87cee634660bed3ac17d9eb5 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 d7f61e7d5909da7227c9e34be06ce9eb872ba074 💫
hebasto:
ACK d7f61e7d5909da7227c9e34be06ce9eb872ba074, I have reviewed the code and it looks OK. Did not make benchmarking though.
Tree-SHA512: 305ac945b4571c5f47646d4f0e78180d7a3d40b2f70ee43e4b3e00c96a465f6d0b9c750b8e85c89ed833e557e2cdb5896743f07ef90e4e53d4ad85452b545886
|
|
4e362c2b7269ae0426010850c605e5c1d0d58234 doc: add release note for 25934 (brunoerg)
fe488b4c4b7aa07fb83d528e2942ef914fd188c0 test: add coverage for `label` in `listsinceblock` (brunoerg)
722e9a418d078ed34aedd1ca55c1ae104f29a7d3 wallet, rpc: add `label` to `listsinceblock` (brunoerg)
852891ff98cffd37a74b9cb96394f43b2e6ca30e 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 4e362c2b7269ae0426010850c605e5c1d0d58234
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/25934/commits/4e362c2b7269ae0426010850c605e5c1d0d58234
aureleoules:
ACK 4e362c2b7269ae0426010850c605e5c1d0d58234
Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737
|
|
libbitcoin_common.a
b19c4124b3c9a15fe3baf8792c55eb26eca51c0f refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky)
dd6e8bd71c6025a51d88000caf28121ec00499db build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake)
82e272a109281f750909d1feade784c778d8b592 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 b19c4124b3c9a15fe3baf8792c55eb26eca51c0f
Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
|
|
fa825bd227b9d2bace896a2d29b5ce78bbd4e59c 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 https://github.com/bitcoin/bitcoin/commit/fa825bd227b9d2bace896a2d29b5ce78bbd4e59c
theStack:
ACK fa825bd227b9d2bace896a2d29b5ce78bbd4e59c
john-moffett:
ACK fa825bd227b9d2bace896a2d29b5ce78bbd4e59c
Tree-SHA512: a7a775718f5f9796b5cffafbb3ace8adb5c163414ec584a57143157fc9dfb86f799e3b9c8365fcb831ee1e9eafc59d699d1653d772c68392de421b3de74dcd61
|
|
|
|
|
|
|
|
|
|
|
|
|
|
5d332da2cfdc67d69a165119c6ff598e29b28afa doc: Drop no longer relevant comment (Hennadii Stepanov)
Pull request description:
The comment was introduced in 4cf3411056f6a59fc5fe07784b6b6a512d76b046, and since 7e4bd19785ff9120b7242ff9f15231868aaf7db4 it has been no longer relevant.
ACKs for top commit:
jarolrod:
ACK 5d332da2cfdc67d69a165119c6ff598e29b28afa
Tree-SHA512: 6d32561336993b1ff7d7c524d090ac52aefb40078ed706ca4c6d5026cc3f63244c49c0e00e45ff192ba0e9f1527faf63249aa18bc8aa677b9e053d387e0f4027
|
|
|
|
|
|
`kernel/mempool_entry.h`
38941a703e079709bb465ad1fcde50e11350f8f1 refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov)
Pull request description:
This PR addresses the https://github.com/bitcoin/bitcoin/pull/17786#discussion_r1027818360:
> why not move it to the right place, that is to `kernel/txmempool_entry.h`?
ACKs for top commit:
MarcoFalke:
review ACK 38941a703e079709bb465ad1fcde50e11350f8f1 📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
|
|
CoinSelection
c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6 test: Check max transaction weight in CoinSelection (Aurèle Oulès)
6b563cae92957dc30dc35103a7c321fdb0115ef3 wallet: Check max tx weight in coin selector (Aurèle Oulès)
Pull request description:
This PR is an attempt to fix #5782.
I have added 4 test scenarios, 3 of them provided here https://github.com/bitcoin/bitcoin/issues/5782#issuecomment-73819058 (slightly modified to use a segwit wallet).
Here are my benchmarks :
## PR
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 1,466,341.00 | 681.97 | 0.6% | 11,176,762.00 | 3,358,752.00 | 3.328 | 1,897,839.00 | 0.3% | 0.02 | `CoinSelection`
## Master
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 1,526,029.00 | 655.30 | 0.5% | 11,142,188.00 | 3,499,200.00 | 3.184 | 1,994,156.00 | 0.2% | 0.02 | `CoinSelection`
ACKs for top commit:
achow101:
reACK c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/25729/commits/c7c7ee9d0b2d7b303b9300f941e37e09e7d8d8b6
furszy:
diff ACK c7c7ee9d
Tree-SHA512: ef0b28576ff845174651ba494aa9adee234c96e6f886d0e032eceb7050296e45b099dda0039d1dfb9944469f067627b2101f3ff855c70353cf39d1fc7ee81828
|
|
203886c443c4ad76f8a1dba740a286e383e55206 Fixup clang-tidy named argument comments (fanquake)
Pull request description:
Fix comments so they are checked/consistent.
Fix incorrect comments.
ACKs for top commit:
hebasto:
ACK 203886c443c4ad76f8a1dba740a286e383e55206, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: e1257840f91fe39842e2b19299c1633604697b8584fe44b1977ada33cdde5433c877ed0b669fa334e20b04971dc89cd47d58b2783b6f7004521f01d05a1245da
|
|
|
|
3eb041f014870954db564369a4be4bd0dea48fbe wallet: Change coin selection fee assert to error (Andrew Chow)
c6e7f224c119f47af250a9e0c5b185cb98b30c4c util: Add StrFormatInternalBug and STR_INTERNAL_BUG (MarcoFalke)
Pull request description:
Returning an error instead of asserting for the low fee check will be better as it does not crash the node and instructs users to report the bug.
ACKs for top commit:
S3RK:
ACK 3eb041f014870954db564369a4be4bd0dea48fbe
aureleoules:
ACK 3eb041f014870954db564369a4be4bd0dea48fbe
furszy:
ACK 3eb041f0
Tree-SHA512: 118c13d7cdfce492080edd4cb12e6d960695377b978c7573f9c58b6d918664afd0e8e591eed0605d08ac756fa8eceed456349de5f3a025174069abf369bb5a5f
|
|
d885bb2f6ea34cd21dacfebe763a07dbb389c1bd test: Test exclusion of OP_RETURN from getblockstats (Fabian Jahr)
ba9d288b2468f047e5a8e4637dd8749e247ff547 test: Fix getblockstats test data generator (Fabian Jahr)
2ca5a496c2f3cbcc63ea15fa05c1658e7f527bbc rpc: Improve getblockstats (Fabian Jahr)
cb94db119f4643f49da63520d64efc99fb0c0795 validation, index: Add unspendable coinbase helper functions (Fabian Jahr)
Pull request description:
Fixes #19885
The genesis block does not have undo data saved to disk so the RPC errored because of that.
ACKs for top commit:
achow101:
ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd
aureleoules:
ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd
stickies-v:
ACK d885bb2f6
Tree-SHA512: f37bda736ed605b7a41a81eeb4bfbb5d2b8518f847819e5d6a090548a61caf1455623e15165d72589ab3f4478252b00e7b624f9313ad6708cac06dd5edb62e9a
|
|
containing legacy key type entries
3198e4239e848bbb119e3638677aa9bcf8353ca6 test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner)
349ed2a0eed3aaaf199ead93057c97730869c3a3 wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner)
Pull request description:
Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details):
```
$ ./src/bitcoin-cli createwallet crashme
$ ./src/bitcoin-cli unloadwallet crashme
$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
SQLite version 3.38.2 2022-03-26 13:51:10
Enter ".help" for usage hints.
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
$ ./src/bitcoin-cli loadwallet crashme
--- bitcoind output: ---
2022-11-06T13:51:01Z Using SQLite Version 3.38.2
2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
2022-11-06T13:51:01Z init message: Loading wallet…
2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900
Segmentation fault (core dumped)
```
Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: https://github.com/bitcoin/bitcoin/blob/50422b770a40f5fa964201d1e99fd6b5dc1653ca/src/wallet/walletdb.cpp#L589-L594
~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~
~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~
This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading.
ACKs for top commit:
achow101:
ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6
aureleoules:
ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6
Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
|
|
Co-authored-by: Andrew Chow <github@achow101.com>
|
|
Co-authored-by: Andrew Chow <github@achow101.com>
|
|
Returning an error instead of asserting for the low fee check will be
better as it does not crash the node and instructs users to report the
bug.
|
|
total amount
7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3 refactor: make CoinsResult total amounts members private (furszy)
3282fad59908da328f8323e1213344fe58ccf69e wallet: add assert to SelectionResult::Merge for safety (S3RK)
c4e3b7d6a154e82cdb902fd7bcb7b725aebde5ea wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy)
cac2725fd0f5baeb741dfe079a87332784c2adc7 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy)
cf793846978a8783c23b66ba6b4f3f30e83ff3eb test: Coin Selection, duplicated preset inputs selection (furszy)
341ba7ffd8cdb56b4cde1f251768c3d2c2a9b4e9 test: wallet, coverage for CoinsResult::Erase function (furszy)
f930aefff9690a1e830d897d0a8c53f4219ae4a8 wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy)
Pull request description:
This comes with #26559.
Solving few bugs inside the wallet's transaction creation
process and adding test coverage for them.
Plus, making use of the `CoinsResult::total_amount` cached value
inside the Coin Selection process to return early if we don't have
enough funds to cover the target amount.
### Bugs
1) The `CoinsResult::Erase` method removes only one
output from the available coins vector (there is a [loop break](https://github.com/bitcoin/bitcoin/blob/c1061be14a515b0ed4f4d646fcd0378c62e6ded3/src/wallet/spend.cpp#L112)
that should have never been there) and not all the preset inputs.
Which on master is not a problem, because since [#25685](https://github.com/bitcoin/bitcoin/pull/25685)
we are no longer using the method. But, it's a bug on v24
(check [#26559](https://github.com/bitcoin/bitcoin/pull/26559)).
This method it's being fixed and not removed because I'm later using it to solve
another bug inside this PR.
2) As we update the total cached amount of the `CoinsResult` object inside
`AvailableCoins` and we don't use such function inside the coin selection
tests (we manually load up the `CoinsResult` object), there is a discrepancy
between the outputs that we add/erase and the total amount cached value.
### Improvements
* This makes use of the `CoinsResult` total amount field to early return
with an "Insufficient funds" error inside Coin Selection if the tx target
amount is greater than the sum of all the wallet available coins plus the
preset inputs amounts (we don't need to perform the entire coin selection
process if we already know that there aren't enough funds inside our wallet).
### Test Coverage
1) Adds test coverage for the duplicated preset input selection bug that we have in v24.
Where the wallet invalidly selects the preset inputs twice during the Coin Selection
process. Which ends up with a "good" Coin Selection result that does not cover the
total tx target amount. Which, alone, crashes the wallet due an insane fee.
But.. to make it worst, adding the subtract fee from output functionality
to this mix ends up with the wallet by-passing the "insane" fee assertion,
decreasing the output amount to fulfill the insane fee, and.. sadly,
broadcasting the tx to the network.
2) Adds test coverage for the `CoinsResult::Erase` method.
------------------------------------
TO DO:
* [ ] Update [#26559 ](https://github.com/bitcoin/bitcoin/pull/26559) description.
ACKs for top commit:
achow101:
ACK 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3
glozow:
ACK 7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3, I assume there will be a followup PR to add coin selection sanity checks and we can discuss the best way to do that there.
josibake:
ACK [7362f8e](https://github.com/bitcoin/bitcoin/pull/26560/commits/7362f8e5e2497bc1ef27bfa871fc6dd306dd33c3)
Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
|
|
Fix comments so they are checked/consistent.
Fix incorrect arguments.
|
|
stale blocks
f39d9269ebbcffe70d02674c67c4f53783b63005 rpc: warn that nodes ignore requests for old stale blocks (Sjors Provoost)
Pull request description:
Adds warning to RPC help that `getblockfrompeer` is of little use for stale blocks that are more than a month old.
This is an anti-fingerprinting measure. See `BlockRequestAllowed` in `net_processing`.
It's been in Bitcoin Core since 2014, introduced in #2910 and later improved to not rely on checkpoints.
Older and alternative clients might still serve these blocks, so not throwing an error.
Allowing whitelisted nodes to fetch these blocks anyway might be nice.
ACKs for top commit:
fjahr:
Code review ACK f39d9269ebbcffe70d02674c67c4f53783b63005
Tree-SHA512: db88f9f7521289640c5e629c840dda1c2c3ab70d458e9e7136c60fbaeb02acfb36dc093502d83d4c098c331e22aab81bf8f4c4961d805e3bde0f8f3cfe68d968
|
|
it from type alias
1984db1d5037646d7ded23d9f26a42524b7e3519 refactor: Rename local variable to distinguish it from type alias (Hennadii Stepanov)
Pull request description:
The `txiter` type alias is declared in the `txmempool.h`: https://github.com/bitcoin/bitcoin/blob/9e59d21fbe5746b220f35b0a5a735198c3e6dcdb/src/txmempool.h#L406
ACKs for top commit:
stickies-v:
ACK 1984db1d5
vasild:
ACK 1984db1d5037646d7ded23d9f26a42524b7e3519
jarolrod:
ACK 1984db1d5037646d7ded23d9f26a42524b7e3519
Tree-SHA512: 127bfb62627e2d79d8cdb0bd0ac11b3737568c3631b54b2d1e37984f673a1f60edf7bc102a269f7eb40e4bb124b910b924a89475c6a6ea978b2171219fa30685
|
|
The comment was introduced in 4cf3411056f6a59fc5fe07784b6b6a512d76b046,
and since 7e4bd19785ff9120b7242ff9f15231868aaf7db4 it has been no longer
relevant.
|
|
|
|
|
|
The CoinsResult class will now count the raw total amount and the effective
total amount internally (inside the 'CoinsResult::Add' and 'CoinsResult::Erase'
methods).
So there is no discrepancy between what we add/erase and the total values.
(which is what was happening on the coinselector_test because the 'CoinsResult'
object is manually created there, and we were not keeping the total amount
in sync with the outputs being added/removed).
|
|
direct member access
Aside from the cleanup, this solves a bug in the following-up commit. Because, in these
tests, we are manually adding/erasing outputs from the CoinsResult object but never
updating the internal total amount field.
|
|
This exercises the bug inside CoinsResult::Erase that
ends up on (1) a wallet crash or (2) a created and
broadcasted tx that contains a reduced recipient's amount.
This is covered by making the wallet selects the preset
inputs twice during the coin selection process.
Making the wallet think that the selection process result covers
the entire tx target when it does not. It's actually creating
a tx that sends more coins than what inputs are covering for.
Which, combined with the SFFO option, makes the wallet
incorrectly reduce the recipient's amount by the difference
between the original target and the wrongly counted inputs.
Which means, a created and relayed tx sending less coins to
the destination than what the user inputted.
|
|
|
|
queued for fully connected peers
8f2dac54096c20afd8fd12c21a2ee5465fea085e [test] Add p2p_tx_privacy.py (dergoegge)
ce63fca13e9b500e9f687d80a457175ac967a371 [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
845e3a34c49abcc634b5a10ccdd6b10fb4fcf449 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)
Pull request description:
`TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake).
Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node.
ACKs for top commit:
MarcoFalke:
ACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e 🔝
jnewbery:
utACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e
Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9
|
|
The `txiter` type alias is declared in the `txmempool.h`.
|
|
|
|
failure cleanup
5e65a216d1fd00c447757736d4f2899d235e731a wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
88afc73ae0c67a4482ecd3d77eb2a8fd2673f82d tests: Test for migrating encrypted wallets (Andrew Chow)
86ef7b3c7be84e4183098f448c77ecc9ea7367ab wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow)
Pull request description:
When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault.
This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached.
This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue.
ACKs for top commit:
S3RK:
reACK 5e65a216d1fd00c447757736d4f2899d235e731a
stickies-v:
ACK [5e65a21](https://github.com/bitcoin/bitcoin/commit/5e65a216d1fd00c447757736d4f2899d235e731a)
furszy:
diff ACK 5e65a21
Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
|
|
ScriptPubKeyMan
1b77db265317a6470d0914b520f04eb64b3c0942 test: add `ismine` test for descriptor scriptpubkeyman (w0xlt)
Pull request description:
Currently `src/wallet/test/ismine_tests.cpp` has tests for the legacy ScriptPubKeyMan only.
This PR adds tests for the descriptor ScriptPubKeyMan.
ACKs for top commit:
ishaanam:
ACK 1b77db265317a6470d0914b520f04eb64b3c0942
achow101:
ACK 1b77db265317a6470d0914b520f04eb64b3c0942
furszy:
ACK 1b77db26 with a non-blocking comment.
Tree-SHA512: 977b5d1e71f9468331aeb4ebaf3708dd651f9f3018d4544a395b87ca6d7fb8bfa6d20acc1a4f6e096e240e81d30fb7a6e8add190e52536e7a3cb5a80f392883f
|
|
|
|
This commit documents our assumption about
TxRelay::m_tx_inventory_to_send being empty prior to version handshake
completion.
The added Assume acts as testing oracle for our fuzzing tests to
potentially detect if the assumption is violated.
|
|
46339d29b10c9fb597af928c21c34945d76bbd22 test, refactor: Reorder sendtxrcncl tests for better readability (Gleb Naumenko)
14263c13f153b84e50191366a6f64f884ed4ddd9 p2p, refactor: Extend logs for unexpected sendtxrcncl (Gleb Naumenko)
87493e112ee91923adf38b75491bedeb45f87c80 p2p, test, refactor: Minor code improvements (Gleb Naumenko)
00c5dec818f60e8297d42b49a919aa82c42821b5 p2p: Clarify sendtxrcncl policies (Gleb Naumenko)
ac6ee5ba211d05869800497d6b518ea1ddd2c718 test: Expand unit and functional tests for txreconciliation (Gleb Naumenko)
bc84e24a4f0736919ea4a76f7d45085587625aba p2p, refactor: Switch to enum class for ReconciliationRegisterResult (Gleb Naumenko)
a60f729e293dcd11ca077b7c1c72b06119437faa p2p: Drop roles from sendtxrcncl (Gleb Naumenko)
6772cbf69cf075ac8dff3507bf9151400ed255b7 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 46339d29b10c9fb597af928c21c34945d76bbd22
ariard:
ACK 46339d2
mzumsande:
Code Review ACK 46339d29b10c9fb597af928c21c34945d76bbd22
Tree-SHA512: b5cc6934b4670c12b7dbb3189e739ef747ee542ec56678bf4e4355bfb481b746d32363c173635685b71969b3fe4bd52b1c8ebd3ea3b35c82044bba69220f6417
|
|
|
|
If migratewallet fails, we do a cleanup which removes the watchonly and
solvables wallets if they were created. However, if they were not, their
pointers are nullptr and we don't check for that, which causes a
segfault during the cleanup. So check that they aren't nullptr before
cleaning them up.
|
|
"checksum_valid" set
13d97608297bd56ed033d0e754d2e50447b02af0 test: load wallet, coverage for crypted keys (furszy)
373c99633ec7f20557db2734c49116ee4ad15423 refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy)
ee7a984f85015b610be4929b7c35cb501c1fbf7c refactor: unify test/util/wallet.h with wallet/test/util.h (furszy)
cc5a5e81217506ec6f9fff34056290f8f40a7396 wallet: bugfix, invalid crypted key "checksum_valid" set (furszy)
Pull request description:
At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.
Note:
The first commit fixes the issue, the two commits in the middle are cleanups so `DuplicateMockDatabase`
can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading
process.
Includes test coverage for the following scenarios:
1) "All ckeys checksums valid" test:
Loads an encrypted wallet with all the crypted keys with a valid checksum and
verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.
(we force a complete ckeys re-write if we find any missing crypted key checksum
during the wallet loading process)
2) "Missing checksum in one ckey" test:
Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
triggers a complete re-write of the crypted keys.
3) "Invalid ckey checksum error" test:
Verifies that loading up a ckey with an invalid checksum stops the wallet loading
process with a corruption error.
4) "Invalid ckey pubkey error" test:
Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
process with a corruption error.
ACKs for top commit:
achow101:
ACK 13d97608297bd56ed033d0e754d2e50447b02af0
aureleoules:
ACK 13d97608297bd56ed033d0e754d2e50447b02af0
Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b
|
|
used together
d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf rpc: Allow named and positional arguments to be used together (Ryan Ofsky)
Pull request description:
It's nice to be able to use named options and positional arguments together.
Most shell tools accept both, and python functions combine options and arguments allowing them to be passed with even more flexibility. This change adds support for python's approach so as a motivating example:
```sh
bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1
```
Can be shortened to:
```sh
bitcoin-cli -named createwallet mywallet load_on_startup=1
```
JSON-RPC standard doesn't have a convention for passing named and positional parameters together, so this implementation makes one up and interprets any unused `"args"` named parameter as a positional parameter array.
This change is backwards compatible. It doesn't change the interpretation of any previously valid calls, just treats some previously invalid calls as valid.
Another use case even if you only occasionally use named arguments is that you can define an alias:
```
alias bcli='bitcoin-cli -named'
```
And now use both named named and unnamed arguments from the same alias without having to manually add `-named` option for named arguments or see annoying error "No '=' in named argument... this needs to be present for every argument (even if it is empty)`" for unnamed arguments
ACKs for top commit:
achow101:
ACK d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf
stickies-v:
re-ACK d8b12a75d
aureleoules:
re-ACK d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf
Tree-SHA512: 0cff8b50f584bcbbd376624adccf40536566ed8d1bcd6c88ad565dbc208f19d5e7a48c994efd6329d42b560149340d330397278f08a2912af5f3418d8c8837a9
|
|
The loop break shouldn't have being there.
|
|
connected peers
|
