fafcc94398 Make bitcoin-util grind_task tsan friendly (MacroFake)
Pull request description:
While there is no issue with the current code, `libtsan-12.2.1` on my machine does not seem to like it. This is understandable, because the nonce isn't protected by a mutex that the sanitizer can see (only by an atomic, which achieves the same).
Fix this by guarding the nonce by the existing atomic bool, which tsan seems to understand.
ACKs for top commit:
ajtowns:
ACK fafcc94398
hebasto:
ACK fafcc94398, I have reviewed the code and it looks OK, I agree it can be merged. Confirming that initial bug has been fixed.
Tree-SHA512: 4e67fab5833ec7d91678b85a300368892ee9f7cd89a52cc5e15a7df65b2da813b24eaffd8362d0d8a3c8951e024041d69ebddf25101b11d0a1a62c1208ddc9a5
564b580bf0 test: Introduce MIN_BLOCKS_TO_KEEP constant (Aurèle Oulès)
71d9a7c03b test: Wallet imports on pruned nodes (Aurèle Oulès)
e6906fcf9e rpc: Enable wallet import on pruned nodes (Aurèle Oulès)
Pull request description:
Reopens #16037
I have rebased the PR, addressed the comments of the original PR and added a functional test.
> Before this change importwallet fails if any block is pruned. This PR makes it possible to importwallet if all required blocks aren't pruned. This is possible because the dump format includes key timestamps.
For reviewers:
`python test/functional/wallet_pruning.py --nocleanup` will generate a large blockchain (~700MB) that can be used to manually test wallet imports on a pruned node. Node0 is not pruned, while node1 is.
ACKs for top commit:
kouloumos:
ACK 564b580bf0
achow101:
reACK 564b580bf0
furszy:
ACK 564b580
w0xlt:
ACK 564b580bf0
Tree-SHA512: b345a6c455fcb6581cdaa5f7a55d79e763a55cb08c81d66be5b12794985d79cd51b9b39bdcd0f7ba0a2a2643e9b2ddc49310ff03d16b430df2f74e990800eabf
Next()'s result is a tri-state - failed, more to go, complete. Replace
the way that this is returned with an enum with values FAIL, MORE, and
DONE rather than with two booleans.
a2724808ab doc: add 23.1 release notes (fanquake)
Pull request description:
Same as past releases / https://github.com/bitcoin/bitcoin/pull/26524 etc.
ACKs for top commit:
stickies-v:
ACK a2724808ab
Tree-SHA512: e9f7ad72c23c8621e8a98ffa0dc0d08ebe30ad0bc8d23e25fabda5b1a9318ff74c65344821267c6af5a8d94c26c775ce83a67cbe0c4922eac07a4319fd94eb49
Copy construction of CBlockIndex objects is a footgun because of the
wide use of equality-by-pointer comparison in the code base. There are
also potential lifetime confusions of using copied instances, since
there are recursive pointer references (e.g. pprev).
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
Delete move constructors and declare the destructor to satisfy the
"rule of 5."
To be eligible for fee-bumping, a transaction must not have any
of its outputs (eg - change) spent in other unconfirmed transactions
in the wallet. However, this check should not apply to abandoned
transactions.
A new test case is added to cover this case.
062e4e9fe9 doc: add 22.1 release notes (fanquake)
Pull request description:
Same as past releases / #26524 etc.
Top commit has no ACKs.
Tree-SHA512: e41b1eaff1aacd89260f070650044629de5673020e0e70bdceb0749981403aad380e5595c494fa5ebaaa7c87e0ebea0def5f5bbd433a4b3b810e40c2de6dc448
fa34e5f3d3 test: Avoid intermittent timeout in feature_assumevalid.py (MarcoFalke)
Pull request description:
Currently the test will spin up p2p connections in the beginning, then announce the headers to all nodes, but only send the blocks sequentially. This takes a long time, so when getting to the last node, it will have already timed out, while node1 is busy eating blocks. Example:
```
node2 2022-12-06T19:31:35.419291Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e (1) peer=0
node2 2022-12-06T19:31:35.424784Z [msghand] [net.cpp:2776] [PushMessage] [net] sending getdata (577 bytes) peer=0
[...]
node2 2022-12-06T19:41:35.423257Z [msghand] [net_processing.cpp:5729] [SendMessages] Timeout downloading block 2cfdb317b3b901f79e2d4f96339d0c0dffd8ef2685d324f62ab0e2fa3402430e from peer=0, disconnecting
node1 2022-12-06T19:41:35.438706Z [msghand] [net_processing.cpp:5783] [SendMessages] [net] Requesting block 6575919043306ed309014d0bd79814b4fab8afaa281e026d8cc3a1c4c2336fbc (1748) peer=0
node2 2022-12-06T19:41:35.521253Z [net] [net.cpp:573] [CloseSocketDisconnect] [net] disconnecting peer=0
node2 2022-12-06T19:41:35.630417Z [net] [net_processing.cpp:1532] [FinalizeNode] [net] Cleared nodestate for peer=0
```
Fix this by only spinning up the p2p connection right before they are needed.
ACKs for top commit:
jamesob:
ACK fa34e5f3d3 ([`jamesob/ackr/26651.1.MarcoFalke.test_avoid_intermittent`](https://github.com/jamesob/bitcoin/tree/ackr/26651.1.MarcoFalke.test_avoid_intermittent))
Tree-SHA512: 7a1b114c07dfa30237c8cd8637dd6646c5c2dc2530c9de61db231738fddc800b620c31dc664237e33d35e951cf161f015fda593162efc9d85c5f68c6e37217d4
Since commit 3340dbadd3 ("Remove
-zapwallettxes"), the `FindWalletTx` helper is only needed to read tx
hashes, so drop the other parameter and rename the method accordingly.
89c1491d35 wallet: if only have one output type, don't perform "mixed" coin selection (furszy)
Pull request description:
For wallets that only have one output type, we are currently performing the same
selection process over the same coins twice.
The "mixed coin selection" doesn't add any value to the result
(there is nothing to mix if the available coins struct has only one type).
ACKs for top commit:
achow101:
ACK 89c1491d35
john-moffett:
ACK 89c1491d35
kristapsk:
cr utACK 89c1491d35
Tree-SHA512: 672eaeed3ba911d13fa61a46f719c8fe1ebe4d2dc7d723040e71937c693659411bc99cdbd9f0014e836b70eebeff1b8ca861f4d81d39e6f79f437364a526edbe
Instead of having DatabaseBatch deal with opening and closing database
cursors, have a separate RAII class that deals with those.
For now, DatabaseBatch manages DatabaseCursor, but this will change
later.
bcb7123406 test: add add_wallet_options to TestShell (josibake)
Pull request description:
following 555519d082, `TestShell` now always runs with `-disablewallet`. simple fix is to add `add_wallet_options` to `add_options`; looks like testshell was overlooked when adding in the `add_wallet_options` call to the functional tests in #26480
ACKs for top commit:
amitiuttarwar:
ACK bcb7123406
Tree-SHA512: db554a8b3c8ff5bd10cab9592b316035a92f86a0a0ae8ff914de9687bbbb6fc2235bdf82c4cd40e4071782f8b6edf91aad372e82ed3b826c9d8ae39dbe3dbf57
e75d227632 Minor fix: Don't directly delete abandoned txes (John Moffett)
Pull request description:
This fully closes bitcoin/bitcoin#12179. Currently, when a user abandons a transaction by clicking "Abandon Transaction" in the context menu, a call is made to remove it from the GUI view:
`model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);`
(The `false` parameter is for `bool showTransaction`)
This behavior is probably unwanted, as the transaction is not actually removed from the wallet and would show up again if the node is restarted.
However, the previous line, `model->wallet().abandonTransaction(hash);`, changes the underlying model and calls `NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);`, which queues a signal that eventually calls back to `updateTransaction`, this time with `showTransaction` set to `true`. This runs on a separate thread, so it gets called *after* the 'subsequent' `updateTransaction`. The transaction gets removed from the GUI and immediately added back.
In a nutshell, `updateTransaction` gets called twice. The first (direct) call deletes the transaction from the GUI. The second (sent via a queued signal) brings it back to the GUI. The first direct call is redundant and unwanted. Worse, if the `abandonTransaction` call fails for any reason, the transaction still gets removed from the GUI. (This is what caused bitcoin#12179. It can still be triggered if, eg., a user clicks "Abandon Transaction" the moment after a new block is found.)
There are no conditions (to my knowledge) where an abandoned transaction should be directly removed from the GUI. If the underlying model changes, the deletion should be reflected anyway by the queued signal to `updateTransaction`.
The behavior is borne out by the QT logs. To reproduce, send a transaction with RBF enabled, then bump the fee, then 'abandon transaction' on the first transaction. The logs will show something like this:
```
2022-11-28T14:48:00Z [qt] GUI: "NotifyTransactionChanged: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 status= 1"
2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
2022-11-28T14:48:00Z [qt] GUI: " inModel=1 Index=381-382 showTransaction=0 derivedStatus=2"
2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
2022-11-28T14:48:00Z [qt] GUI: " inModel=0 Index=381-381 showTransaction=1 derivedStatus=0"
```
Notice the duplicate `updateWallet` calls with different `showTransaction` values.
ACKs for top commit:
hebasto:
ACK e75d227632
jarolrod:
tACK e75d227632
Tree-SHA512: 00f150f747c2ee1605af861a21d5c3b9773a4a9985e8dab62e48bd32885b1bfa4e8cbf805ad61af77aec9d3ccefaed3f4311a29086aa8c22d55d5326ba68ece6
If a wallet has key birthdates that are more recent than the currrent
chain tip, or a bestblock height higher than the current tip, we should
not attempt to rescan as there is nothing to scan for.
* Use SECP256K1_CONTEXT_NONE when creating signing context, as
SECP256K1_CONTEXT_SIGN is deprecated and unnecessary.
* Use secp256k1_static_context where applicable.
798430d127 wallet: Sanity check fee paid cannot be negative (Andrew Chow)
c1a84f108e wallet: Move fee underpayment check to after fee setting (Andrew Chow)
e5daf976d5 wallet: Rename nFeeRet in CreateTransactionInternal to current_fee (Andrew Chow)
Pull request description:
Currently the fee underpayment check occurs right after we calculate what the transaction's fee should be. However the fee paid by the transaction at that time does not always match. Notably, when doing SFFO, the fee paid at that time will almost always be less than the fee required, which then required having a bypass of the underpayment check that results in SFFO payments going through when they should not.
This PR moves the underpayment check to after fees have been finalized so that we always check whether the fee is being underpaid. This removes the exception for SFFO and unifies this behavior for both SFFO and non-SFFO txs.
ACKs for top commit:
S3RK:
Code review ACK 798430d127
furszy:
Code review ACK 798430d
glozow:
utACK 798430d127, code looks correct to me
Tree-SHA512: 720e8a3dbdc9937b12ee7881eb2ad58332c9584520da87ef3080e6f9d6220ce8d3bd8b9317b4877e56a229113437340852976db8f64df0d5cc50723fa04b02f0
8c3ff7d52a test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563825 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8c rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e67 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52a 🗂
kristapsk:
ACK 8c3ff7d52a
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
When CalculateMemPoolAncestors fails unexpectedly (e.g. it exceeds
ancestor/descendant limits even though we expect no limits to be applied),
add an error log entry for increased visibility. For debug builds,
the application will even halt completely since this is not supposed
to happen.
There are quite a few places that assume CalculateMemPoolAncestors
will return a value without raising an error. This helper function
adds logging (and Assume for debug builds) that ensures robustness
but increases visibility in case of unexpected failures
