38941a703e 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 38941a703e📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
203886c443 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 203886c443, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: e1257840f91fe39842e2b19299c1633604697b8584fe44b1977ada33cdde5433c877ed0b669fa334e20b04971dc89cd47d58b2783b6f7004521f01d05a1245da
3eb041f014 wallet: Change coin selection fee assert to error (Andrew Chow)
c6e7f224c1 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 3eb041f014
aureleoules:
ACK 3eb041f014
furszy:
ACK 3eb041f0
Tree-SHA512: 118c13d7cdfce492080edd4cb12e6d960695377b978c7573f9c58b6d918664afd0e8e591eed0605d08ac756fa8eceed456349de5f3a025174069abf369bb5a5f
d885bb2f6e test: Test exclusion of OP_RETURN from getblockstats (Fabian Jahr)
ba9d288b24 test: Fix getblockstats test data generator (Fabian Jahr)
2ca5a496c2 rpc: Improve getblockstats (Fabian Jahr)
cb94db119f 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 d885bb2f6e
aureleoules:
ACK d885bb2f6e
stickies-v:
ACK d885bb2f6
Tree-SHA512: f37bda736ed605b7a41a81eeb4bfbb5d2b8518f847819e5d6a090548a61caf1455623e15165d72589ab3f4478252b00e7b624f9313ad6708cac06dd5edb62e9a
3198e4239e test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner)
349ed2a0ee 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: 50422b770a/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 3198e4239e
aureleoules:
ACK 3198e4239e
Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
7362f8e5e2 refactor: make CoinsResult total amounts members private (furszy)
3282fad599 wallet: add assert to SelectionResult::Merge for safety (S3RK)
c4e3b7d6a1 wallet: SelectCoins, return early if wallet's UTXOs cannot cover the target (furszy)
cac2725fd0 test: bugfix, coinselector_test, use 'CoinsResult::Erase/Add' instead of direct member access (furszy)
cf79384697 test: Coin Selection, duplicated preset inputs selection (furszy)
341ba7ffd8 test: wallet, coverage for CoinsResult::Erase function (furszy)
f930aefff9 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](c1061be14a/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 7362f8e5e2
glozow:
ACK 7362f8e5e2, 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](7362f8e5e2)
Tree-SHA512: 37a6828ea10d8d36c8d5873ceede7c8bef72ae4c34bef21721fa9dad83ad6dba93711c3170a26ab6e05bdbc267bb17433da08ccb83b82956d05fb16090328cba
f39d9269eb 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 f39d9269eb
Tree-SHA512: db88f9f7521289640c5e629c840dda1c2c3ab70d458e9e7136c60fbaeb02acfb36dc093502d83d4c098c331e22aab81bf8f4c4961d805e3bde0f8f3cfe68d968
1984db1d50 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`: 9e59d21fbe/src/txmempool.h (L406)
ACKs for top commit:
stickies-v:
ACK 1984db1d5
vasild:
ACK 1984db1d50
jarolrod:
ACK 1984db1d50
Tree-SHA512: 127bfb62627e2d79d8cdb0bd0ac11b3737568c3631b54b2d1e37984f673a1f60edf7bc102a269f7eb40e4bb124b910b924a89475c6a6ea978b2171219fa30685
MarcoFalke reported the case of positional arguments silently overwriting the
named "args" parameter in bitcoin-cli
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471 and this
behavior is confusing and was not intended when support for "args" parameters
was added to bitcoin-cli in #19762.
Instead of letting one "args" value overwrite the other in the client, just
pass the values to the server verbatim, and let the error be handled server
side.
Specifying same named parameter multiple times is still allowed by bitcoin-cli.
The client implementation overwrites earlier option values with later ones
before sending to server. This is tested by interface_bitcoin_cli.py
Rationale for allowing client parameters to be specified multiple times in
bitcoin-cli is that this behavior has been supported for a long time, and that
when using the command line interactively, it can be convenient to override
earlier option values with new values without having to go back and remove the
old value.
But for the RPC server, there isn't really a good use-case for earlier values
to be discarded if multiple values are specified. JSON keys are generally
supposed to be unique and if they aren't it's probably an indication of some
problem generating the RPC request.
Current behavior isn't ideal and will be changed in upcoming commits, but it's
useful to have test coverage regardless.
MarcoFalke reported the case of bitcoin-cli positional arguments overwriting
the named "args" parameter in
https://github.com/bitcoin/bitcoin/pull/19762#discussion_r1035761471
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).
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.
8f2dac5409 [test] Add p2p_tx_privacy.py (dergoegge)
ce63fca13e [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
845e3a34c4 [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 8f2dac5409🔝
jnewbery:
utACK 8f2dac5409
Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9
5e65a216d1 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
88afc73ae0 tests: Test for migrating encrypted wallets (Andrew Chow)
86ef7b3c7b 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 5e65a216d1
stickies-v:
ACK [5e65a21](5e65a216d1)
furszy:
diff ACK 5e65a21
Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
1b77db2653 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 1b77db2653
achow101:
ACK 1b77db2653
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.
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
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.
13d9760829 test: load wallet, coverage for crypted keys (furszy)
373c99633e refactor: move DuplicateMockDatabase to wallet/test/util.h (furszy)
ee7a984f85 refactor: unify test/util/wallet.h with wallet/test/util.h (furszy)
cc5a5e8121 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 13d9760829
aureleoules:
ACK 13d9760829
Tree-SHA512: 9ea630ee4a355282fbeee61ca04737294382577bb4b2631f50e732568fdab8f72491930807fbda58206446c4f26200cdc34d8afa14dbe1241aec713887d06a0b
d8b12a75db 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 d8b12a75db
stickies-v:
re-ACK d8b12a75d
aureleoules:
re-ACK d8b12a75db
Tree-SHA512: 0cff8b50f584bcbbd376624adccf40536566ed8d1bcd6c88ad565dbc208f19d5e7a48c994efd6329d42b560149340d330397278f08a2912af5f3418d8c8837a9
These belong in libbitcoin_common.a, not libbitcoin_util.a, because they aren't
general-purpose utilities, they just contain common code that is used by both
the node and the wallet. Another reason to reason to not include these in
libbitcoin_util.a is to prevent them from being used by the kernel library.
There is no situation in which CNodeStateStats could be
missing for a legitimate reason - this can only happen if
there is a race condition between peer disconnection and
the getpeerinfo call, in which case the disconnected peer
doesn't need to be included in the response.
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
This fully closes bitcoin#12179. Currently, in the GUI, when a user
abandons a transaction, a call is made to remove it from the list,
and another signal fires (eventually) that adds it back to the GUI
with a trash can icon.
There are no conditions where the abandoned transaction should be
directly removed from the GUI. If the underlying model changes, the
deletion will be reflected anyway.
fa3b2cf277 fuzz: Move-only net utils (MarcoFalke)
Pull request description:
This should speed up fuzz builds when `src/test/fuzz/util.h` is modified. Also, it makes sense on its own.
ACKs for top commit:
dergoegge:
ACK fa3b2cf277
Tree-SHA512: 03d6abeb728ac8eb3f28167e8ac43d8d6e7e1b1738ec14f58a36e17502081fdde2d56f2d47a9e11b991754667e83b2eb22d154e394c0c1c4ffa0945db86b7e21
This code was a bit hard to understand, so make it less dense and
add more explanations. Doesn't change behavior.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
