47c4b1f52a mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v)
5481f65849 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v)
f911bdfff9 mempool: use util::Result for CalculateMemPoolAncestors (stickies-v)
66e028f739 mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v)
Pull request description:
Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear.
## Unexpected behaviour
This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs.
In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit.
## Improvements
### Return value instead of out-parameters
This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit.
### Observability
There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](69b10212ea/src/node/miner.cpp (L399)). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here.
ACKs for top commit:
achow101:
ACK 47c4b1f52a
w0xlt:
ACK 47c4b1f52a
glozow:
ACK 47c4b1f52a
furszy:
light code review ACK 47c4b1f5
aureleoules:
ACK 47c4b1f52a
Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648
04609284ad rpc: Improve error when wallet is already loaded (Aurèle Oulès)
Pull request description:
Currently, trying to load a descriptor (sqlite) wallet that is already loaded throws the following error:
> error code: -4
> error message:
> Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?
I don't think it is very clear what it means for a user.
While a legacy wallet would throw:
> error code: -35
> error message:
> Wallet file verification failed. Refusing to load database. Data file '/home/user/.bitcoin/signet/wallets/test_wallet/wallet.dat' is already loaded.
This PR changes the error message for both types of wallet to:
> error code: -35
> error message:
> Wallet file verification failed. Wallet "test_wallet" is already loaded.
ACKs for top commit:
achow101:
ACK 04609284ad
hernanmarino:
ACK 0460928
theStack:
Tested ACK 04609284ad
Tree-SHA512: a8f3d5133bfaef7417a6c05d160910ea08f32ac62bfdf7f5ec305ff5b62e9113b55f385abab4d5a4ad711aabcb1eb7ef746eb41f841b196e8fb5393ab3ccc01e
9622fe64b8 test: move coins result test to wallet_tests.cpp (furszy)
f69347d058 test: extend and simplify availablecoins_tests (furszy)
212ccdf2c2 wallet: AvailableCoins, add arg to include/skip locked coins (furszy)
Pull request description:
Negative PR with extended test coverage :).
1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`.
So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`.
3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount.
Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.
ACKs for top commit:
achow101:
ACK 9622fe64b8
theStack:
ACK 9622fe64b8
aureleoules:
reACK 9622fe64b8, nice cleanup!
Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388
81d4a2b14f refactor: Move feerate comparison invariant outside of the loop (yancy)
365aca4045 refactor: Simplify feerate comparison statement (yancy)
Pull request description:
This is a small nit, however I think it's more understandable to write:
`utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee`
vs
`(utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0`
ACKs for top commit:
Xekyo:
ACK 81d4a2b14f
achow101:
ACK 81d4a2b14f
aureleoules:
ACK 81d4a2b14f
Tree-SHA512: 3e89377989c36716b53114fe40178261671dde5688075fab1c21ec173ac310f8c84ed6af90354d7c329176cb7262dfcaa7191fd19847d3b7147a9a10c3e31176
f496528556 walletdb: refactor: drop unused `FindWalletTx` parameter and rename (Sebastian Falbesoner)
Pull request description:
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.
ACKs for top commit:
S3RK:
code review ACK f496528556
achow101:
ACK f496528556
vincenzopalazzo:
ACK f496528556
Tree-SHA512: ead85bc724462f9e920f9d7fe89679931361187579ffd6e63427c8bf5305cd5f71da24ed84f3b1bd22a12be46b5abec13f11822e71a3e1a63bf6cf49de950ab5
The field 'comment' appears twice in TransactionDescriptionString,
incorrectly - this commit removes the instance of the comment field
without a description, preserving the one with a description
f1e89597c8 test: Drop no longer required bench output redirection (Hennadii Stepanov)
4dbcdf26a3 bench: Suppress output when running with `-sanity-check` option (Hennadii Stepanov)
Pull request description:
This change allows to simplify CI tests, and makes it easier to integrate the `bench_bitcoin` binary into CMake custom [targets](https://cmake.org/cmake/help/latest/command/add_custom_target.html) or [commands](https://cmake.org/cmake/help/latest/command/add_custom_command.html), as `COMMAND` does not support output redirection.
ACKs for top commit:
aureleoules:
tACK f1e89597c8. Ran as expected and is more practical than using an output redirection.
Tree-SHA512: 29086d428cccedcfd031c0b4514213cbc1670e35f955e8fd35cee212bc6f9616cf9f20d0cb984495390c4ae2c50788ace616aea907d44e0d6a905b9dda1685d8
b2aa9e8528 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders)
8c5b3646b5 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders)
Pull request description:
Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2)
was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
Related mailing list discussions here:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html
And a couple years earlier:
https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html
ACKs for top commit:
achow101:
reACK b2aa9e8528
glozow:
reACK b2aa9e8528
pablomartin4btc:
re-ACK b2aa9e8528
jonatack:
ACK b2aa9e8528 with some suggestions
Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad
bb5ea1d9a9 qt: Load PSBTs using istreambuf_iterator rather than istream_iterator (Andrew Chow)
Pull request description:
`istream_iterator` eats whitespace charactesr which causes parsing failures for PSBTs that contain the bytes corresponding to those characters. `istreambuf_iterator` is the correct thing to use here.
This is a regression in 24.0. https://github.com/bitcoin/bitcoin/pull/25001 accidentally changed the original `istreambuf_iterator` to `istream_iterator`.
ACKs for top commit:
furszy:
Tested ACK bb5ea1d9
MarcoFalke:
review ACK bb5ea1d9a9🍇
Tree-SHA512: 35d90eee3efdcb6a360af69ac1727f9f2837ea621297196de3136299f5de6d9975df4e425e1fc5b8813c1ddb2a4d60c3969e1d5d968953a4628ca45e37d3bf05
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.
There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.
Two changes could be accomplished:
1) Anything not 64 bytes could be allowed
2) Anything above 64 bytes could be allowed
In the Great Consensus Cleanup, suggestion (2) was the route taken.
It would not allow an "empty" OP_RETURN
but would reduce the required padding from 22 bytes to 5.
The functional test is also modified to test the actual case
we care about: 64 bytes
6fefd49527 rpc: Require NodeStateStats object in getpeerinfo (Martin Zumsande)
Pull request description:
The objects `CNode`, `CNodeState` and `Peer` store different info about a peer - `InitializeNode()` and `FinalizeNode()` make sure that for the duration of a connection, we should always have one of each for a peer.
Therefore, there is no situation in which, as part of getpeerinfo RPC, `GetNodeStateStats()` (which requires a `CNodeState` and a `Peer` entry for a `NodeId` to succeed) could fail for a legitimate reason while the peer is connected - this can only happen if there is a race condition between peer disconnection and the `getpeerinfo` processing (see also a more detailed description of this in https://github.com/bitcoin/bitcoin/pull/26457#pullrequestreview-1181641835).
But in this case I think it's better to just not include the newly disconnected peer in the response instead of returning just parts of its data.
An earlier version of this PR also made the affected `CNodeStateStats` fields non-optional (see 5f900e27d0). Since this conflicts with #25923 and should be a separate discussion, I removed that commit from this PR.
ACKs for top commit:
dergoegge:
Approach ACK 6fefd49527
MarcoFalke:
review ACK 6fefd49527👒
Tree-SHA512: 89c8f7318df4634c1630415de9c8350e6dc2d14d9d07e039e5b180c51bfd3ee2ce99eeac4f9f858af7de846f7a6b48fcae96ebac08495b30e431a5d2d4660532
36c201feb7 remove CBlockIndex copy construction (James O'Beirne)
Pull request description:
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 members (e.g. pprev).
(See also https://github.com/bitcoin/bitcoin/pull/24008#discussion_r891949166)
We can't just delete the copy constructors because they are used for
derived classes (CDiskBlockIndex), so we mark them protected.
ACKs for top commit:
ajtowns:
ACK 36c201feb7 - code review only
MarcoFalke:
re-ACK 36c201feb7 🏻
Tree-SHA512: b1cf9a1cb992464a4377dad609713eea63cc099435df374e4553bfe62d362a4eb5e3c6c6649177832f38c0905b23841caf9d62196cef8e3084bfea0bfc26374b
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
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."
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
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
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
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.
