bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb test: doc: improve doc for `from_hex` helper (mention `to_hex` alternative) (Sebastian Falbesoner)
191405420815d49ab50184513717a303fc2744d6 scripted-diff: test: rename `FromHex` to `from_hex` (Sebastian Falbesoner)
a79396fe5f8f81c78cf84117a87074c6ff6c9d95 test: remove `ToHex` helper, use .serialize().hex() instead (Sebastian Falbesoner)
2ce7b47958c4a10ba20dc86c011d71cda4b070a5 test: introduce `tx_from_hex` helper for tx deserialization (Sebastian Falbesoner)
Pull request description:
There are still many functional tests that perform conversions from a hex-string to a message object (deserialization) manually. This PR identifies all those instances and replaces them with a newly introduced helper `tx_from_hex`.
Instances were found via
* `git grep "deserialize.*BytesIO"`
and some of them manually, when it were not one-liners.
Further, the helper `ToHex` was removed and simply replaced by `.serialize().hex()`, since now both variants are in use (sometimes even within the same test) and using the helper doesn't really have an advantage in readability. (see discussion https://github.com/bitcoin/bitcoin/pull/22257#discussion_r652404782)
ACKs for top commit:
MarcoFalke:
review re-ACK bdb8b9a347e68f80a2e8d44ce5590a2e8214b6bb 😁
Tree-SHA512: e25d7dc85918de1d6755a5cea65471b07a743204c20ad1c2f71ff07ef48cc1b9ad3fe5f515c1efaba2b2e3d89384e7980380c5d81895f9826e2046808cd3266e
b9e76f1bf08c52fcd402b2314e00db4ad247ebc8 rpc: Add test for -rpcwaittimeout (Christian Decker)
f76cb10d7dc9a7b0c55d28011161606399417664 rpc: Prefix rpcwaittimeout error with details on its nature (Christian Decker)
c490e17ef698a1695050f82ef6567b3b87a21861 doc: Add release notes for the `-rpcwaittimeout` cli parameter (Christian Decker)
a7fcc8eb59fe51473571661316214156fbdbdcae rpc: Add a `-rpcwaittimeout` parameter to limit time spent waiting (Christian Decker)
Pull request description:
Adds a new numeric `-rpcwaittimeout` that can be used to limit the
time we spend waiting on the RPC server to appear. This is used by
downstream projects to provide a bit of slack when `bitcoind`s RPC
interface is not available right away.
This makes the `-rpcwait` argument more useful, since we can now limit
how long we'll ultimately wait, before potentially giving up and reporting
an error to the caller. It was discussed in the context of the BTCPayServer
wanting to have c-lightning wait for the RPC interface to become available
but still have the option of giving up eventually ([4355]).
I checked with laanwj whether this is already possible ([comment]), and
whether this would be a welcome change. Initially I intended to repurpose
the (optional) argument to `-rpcwait`, however I decided against it since it
would potentially break existing configurations, using things like `rpcwait=1`,
or `rpcwait=true` (the former would have an unintended short timeout, when
old behavior was to wait indefinitely).
~Due to its simplicity I didn't implement a test for it yet, but if that's desired I
can provide one.~ Test was added during reviews.
[4355]: https://github.com/ElementsProject/lightning/issues/4355
[comment]: https://github.com/ElementsProject/lightning/issues/4355#issuecomment-768288261
ACKs for top commit:
laanwj:
Code review ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8
promag:
ACK b9e76f1bf08c52fcd402b2314e00db4ad247ebc8.
Tree-SHA512: 3cd6728038ec7ca7c35c2e7ccb213bfbe963f99a49bb48bbc1e511c4dd23d9957c04f9af1f8ec57120e47b26eaf580b46817b099d5fc5083c98da7aa92db8638
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1f449586a9e39bc4fb53cb5c7a31362e47aea19b test: add `bad-txns-prevout-null` test to mempool_accept.py (Sebastian Falbesoner)
aa0a5bb70d77739d43d5a9ceae78fb0c6fafd435 test: add `bad-txns-prevout-null` test case to invalid_txs.py (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing tests for the reject reason `bad-txns-prevout-null`, which is thrown in the function `CheckTransaction()`: a62fc35a15/src/consensus/tx_check.cpp (L52-L54)
Basically this condition is met for non-coinbase transactions (the code snippet above only hits if `!tx.IsCoinBase()`) with coinbase-like outpoints, i.e. hash=0, n=0xffffffff.
Can be tested by running the functional tests `feature_block.py`, `p2p_invalid_tx.py` and `mempool_accept.py`. Not sure if the redundancy in the tests is desired (I guess it would make sense if the mempool acceptance test also makes use of the invalid_txs templates?).
ACKs for top commit:
rajarshimaitra:
tACK 1f449586a9
brunoerg:
tACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b
kristapsk:
ACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b, code looks correct and all tests pass.
Tree-SHA512: 2d4f940a6ac8e0d80d2670c9e1111cbf43ae6ac62809a2ccf17cffee9a41d387ea4d889ee300eb4a407c055b13bfa5d37102a32ed59964a9b6950bd907ba7204
fa6847d2fb refactor: drop circular dependencies over deterministicmns in validationinterface (Konstantin Akimov)
f8d1853903 refactor: even more passing CDeterministicMNManager by ref (Kittywhiskers Van Gogh)
d4aa891735 refactor: more passing CDeterministicMNManager by ref (Kittywhiskers Van Gogh)
e628d7517a refactor: pass CDeterministicMNManager by ref to CJContext members (Kittywhiskers Van Gogh)
d731f4127e refactor: pass CDeterministicMNManager by ref to LLMQContext members (Kittywhiskers Van Gogh)
6bd23f40aa refactor: pass CDeterministicMNManager by ref to CGovernanceManager (Kittywhiskers Van Gogh)
055dbba1fa refactor: move GetListAtChainTip() calls out of llmq::utils::*, misc changes (Kittywhiskers Van Gogh)
da39b73f01 refactor: move GetListAtChainTip() calls out of CGovernanceVote (Kittywhiskers Van Gogh)
e9de972982 refactor: move GetListAtChainTip() calls out of CGovernanceObject (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/5929
## Breaking Changes
None. Changes are limited to refactoring, no logical changes have been made.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
(self) utACK fa6847d2fb
knst:
utACK fa6847d2fb
Tree-SHA512: f1acf44bad25789d03c48788a3ac1807d7c553ee45164276ffa6f0dc39dc02e5d29d364469633aa84b341e8c4c13508e0e7a114de6e01fb624f256acfd97199b
140e91fdca merge bitcoin#20864: Move SocketSendData lock annotation to header (Kittywhiskers Van Gogh)
e432122cbd net: Move CConnman/NetEventsInterface after CNode in header file (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Broken off from upcoming backport PR relating to networking due to primarily move-only large diff.
Review large-diff commit using `git diff develop..e432122cbd1cc9bec45cc01c3eb4194a05af1d0e --color-moved=dimmed-zebra --patience` ([source](fa0a71781a))
## Breaking Changes
None. Changes are primarily move-only with minor changes.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 140e91fdca
Tree-SHA512: f2a42532819085aa8d0908b7b102f8d777681d247cd5cecf7f46f71435df2408f39fddd52382ed55ccee686a185092b0dd7f5029b2610be56d3a57b309466238
5a6b8b6b1f partial Merge bitcoin/bitcoin#27053: wallet: reuse change dest when re-creating TX with avoidpartialspends (fanquake)
2f788aa76d fix: change port to use for zmq in interface_zmq_dash.py (Konstantin Akimov)
0ce66fd477 Merge #19507: Expand functional zmq transaction tests (Wladimir J. van der Laan)
a1c2386153 Merge #17445: zmq: Fix due to invalid argument and multiple notifiers (Wladimir J. van der Laan)
44929bad82 Merge #16404: qa: Test ZMQ notification after chain reorg (MarcoFalke)
1707f01309 fix: follow-up changes from bitcoin/bitcoin#22220 for maxapsfee (Konstantin Akimov)
eb4270deae Merge #19743: -maxapsfee follow-up (Samuel Dobson)
6a6d379711 Merge #19756: tests: add sync_all to fix race condition in wallet groups test (MarcoFalke)
5821a1d23a Merge #14582: wallet: always do avoid partial spends if fees are within a specified range (Samuel Dobson)
59d5a4ef39 Merge #19773: wallet: Avoid recursive lock in IsTrusted (Samuel Dobson)
2489f29f0e Merge #19830: test: Add tsan supp for leveldb::DBImpl::DeleteObsoleteFiles (fanquake)
10fa7a66b6 Merge #19538: ci: Add tsan suppression for race in DatabaseBatch (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v21
## What was done?
- bitcoin/bitcoin#19538
- bitcoin/bitcoin#19830
- bitcoin/bitcoin#19773
- bitcoin/bitcoin#14582
- bitcoin/bitcoin#19756
- bitcoin/bitcoin#19743
- bitcoin/bitcoin#16404
- bitcoin/bitcoin#17445
- bitcoin/bitcoin#19507
- partial bitcoin/bitcoin#27053
+some extra fixes and missing changes from bitcoin/bitcoin#22220 for `maxapsfee`
+changed port for zmq in `interface_zmq_dash.py` to prevent intermittent error in `interface_zmq.py`
## How Has This Been Tested?
Run unit & functional tests
## Breaking Changes
`CreateTransaction` now uses sometime 2 private keys for one transaction instead one
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 5a6b8b6b1f
Tree-SHA512: 7efd8a31808f155c08035d0fb7ceaac369e3e44e68d2c91a88e52815a60efba5fe9458f41d93352627c2c062d414fb0207dcf216fa75b54af210b503f9123de6
991c9ec1a2 refactor: accept NodeContext arg into BlockAssembler instead of managers (Kittywhiskers Van Gogh)
c427305805 refactor: remove CNetFulfilledRequestManager global, move to NodeContext (Kittywhiskers Van Gogh)
a53046c4a9 refactor: remove CDSTXManager global and alias, move to CJContext (Kittywhiskers Van Gogh)
f0451fb98d refactor: remove CCreditPoolManager global, move to NodeContext (Kittywhiskers Van Gogh)
67cfee70f8 refactor: remove CMNHFManager::GetSignalsStage alias from CChainState (Kittywhiskers Van Gogh)
667852c851 refactor: cleanup CDSNotificationInterface member names, add asserts (Kittywhiskers Van Gogh)
0d6f736a19 fix: add missing entity for destruction in DashTestSetupClose (Kittywhiskers Van Gogh)
2dd6082e54 refactor: move special transaction processing into helper class (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/5908
## Breaking Changes
None. Changes are limited to refactoring, no logical changes have been made.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 991c9ec1a2
Tree-SHA512: 7a8d90416b6de4915639908b5f6bd59caee73e26462864d12a14a5d74af300a3733d8d9b0863e267b82d2807a8a5753acf5574306a0493bb7f3d83d8f0a7da4f
c9ffb72fb5 fix: avoid `hSocket` double lock (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
It's is locked in `CloseSocketDisconnect()` already.
To be merged before #5511 or 19915 backport is going to cause issues otherwise.
## What was done?
Assert the lock is held already, instead of locking it again.
## How Has This Been Tested?
Run tests, run a node on testnet and drop connections to peers
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK c9ffb72fb5
Tree-SHA512: 6042d0683bf9b1326a74d73b5b44896a0470865b88e1c686d6eefe55c0d419b11a134474499bb6c9c308af69b2f7c4a60070d4535072304330cb640f91f5e367
Decoupling initialization from loading the database means we need to
assert if the database is actually loaded before performing any operations
on it (i.e. check if the manager is "valid")
c5031685bc fix: rename arguments for 'voteraw' (Konstantin Akimov)
3621966f12 feat: add todo to drop Throw() from rpc/util.h (Konstantin Akimov)
b54f03a0c1 fix: wrong name of argument for coinjoin (Konstantin Akimov)
d0163543d9 refactor: use new format CPCCommand for rpc/coinjoin (Konstantin Akimov)
0e1a31159f Merge #19994: Assert that RPCArg names are equal to CRPCCommand ones (net, rpcwallet) (MarcoFalke)
af9eb81e56 fix: wrong name of arguments for RPC (Konstantin Akimov)
c30c8f22dd Merge #19849: Assert that RPCArg names are equal to CRPCCommand ones (blockchain,rawtransaction) (MarcoFalke)
f525f574b0 fix: follow-up missing changes from Merge #18607: rpc: Fix named arguments in documentation (Konstantin Akimov)
7ac1ee0fb4 Merge #19717: rpc: Assert that RPCArg names are equal to CRPCCommand ones (mining,zmq,rpcdump) (MarcoFalke)
860d31f504 Merge #19455: rpc generate: print useful help and error message (Wladimir J. van der Laan)
41c35fd8dc fix: adjust missing arguments and help for misc rpc: debug, echo, mnsync (Konstantin Akimov)
58d923cd5b Merge #19528: rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
This batch of backports asserts that RPCArg names are equal to CRPCCommand ones.
## What was done?
done backports:
- bitcoin/bitcoin#19994
- bitcoin/bitcoin#19849
- bitcoin/bitcoin#19717
- bitcoin/bitcoin#19455
- bitcoin/bitcoin#19528
Beside that same changes are applied for src/coinjoin's rpc.
There's also applied multiple fixes for various rpcs for cases when RPCArg names are mismatched with CPCCommand
**Please, note, that this PR is not final fix for all RPCArgs**. There's a lot of dash's rpc that is not refactored that.
That it is not easy to implement for `quorum command` because the list of arguments (and even their numbers) are different for each sub-command. This fixes are out-of scope of this PR and should be done before bitcoin#18531 is backported.
See also relevant bitcoin#21035.
## How Has This Been Tested?
I used this helper to see which exactly args are specified wrongly:
```cpp
diff --git a/src/rpc/server.h b/src/rpc/server.h
index d4a7ba60eb..cdfd741f54 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -16,6 +16,7 @@
#include <string>
#include <univalue.h>
+#include <logging.h>
class CRPCCommand;
@@ -110,6 +111,19 @@ public:
fn().GetArgNames(),
intptr_t(fn))
{
+ if (fn().m_name != name_in || fn().GetArgNames() != args_in) {
+ std::cerr << "names: " << fn().m_name << ' ' << name_in << std::endl;
+ std::cerr << "arg names: " << fn().GetArgNames().size() << std::endl;
+ for (const auto& i : fn().GetArgNames()) {
+ std::cerr << "arg: " << i << std::endl;
+ }
+ std::cerr << "FIASCO FIASCO FIASCO FIASCO FIASCO FIASCO" << std::endl;
+ }
CHECK_NONFATAL(fn().m_name == name_in);
CHECK_NONFATAL(fn().GetArgNames() == args_in);
}
```
## Breaking Changes
N/A
Some arguments are renamed in RPC but they have been broken (used incorrect name not same as in docs)
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
(self)utACK c5031685bc
Tree-SHA512: e885a8f8fa8bc282dae092fe8df65a37e2ab6ca559cd0598d54bfc06cacddb3bd6f3c74fa2d9c1551f8a4fbdfdeabb8d065649df66d5809e792aec6f51d0df14
86dd0eabb4 chore: increase amount of build jobs from 4 to 8 for depends (Konstantin Akimov)
Pull request description:
## What was done?
Bump CI yaml to use 8 threads for build depends
## How Has This Been Tested?
Builds logs says:
```
$ make -j$(nproc) -C depends HOST=$HOST $DEP_OPTS
make: Entering directory '/builds/dashpay/dash/depends'
```
https://gitlab.com/dashpay/dash/-/jobs/6409522172
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 86dd0eabb4
Tree-SHA512: dfdcae5473a5315d3fc64721f432d671a14cab9811305334f948a73ddc13e5f3e58be433988030ef6019cddd3f383c87722eaed40d8b3ab570e5c224c20191e0
dfddfd09a4 trivial: remove unneeded header, enumerate circular dependencies (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
As found in https://github.com/dashpay/dash/pull/5929#discussion_r1526663050, `validation.h` is not needed for `specialtxman.cpp` and removing the header uncovers other circular dependencies that were obscured by the shortest circular path (see https://github.com/dashpay/dash/pull/5929#discussion_r1527594636).
## Breaking Changes
None.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK dfddfd09a4
Tree-SHA512: c3e74c1a381c0b69985a29f57da3565ec7681548389f09ab8b9907386f0a9f221db92a39409eb46595f3546edffa389774b96b0c9dd8f4fedff46f2d3bd635f1
14b4921a91920df25b19ff420bfe2bff8c56f71e wallet: reuse change dest when recreating TX with avoidpartialspends (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27051
When the wallet creates a transaction internally, it will also create an alternative that spends using destination groups and see if the fee difference is negligible. If it costs the user the same to send the grouped version, we send it (even if the user has `avoidpartialspends` set to `false` which is default). This patch ensures that the second transaction creation attempt re-uses the change destination selected by the first attempt. Otherwise, the first change address remains reserved, will not be used in the second attempt, and then will never be used by the wallet, leaving gaps in the BIP44 chain.
If the user had `avoidpartialspends` set to true, there is no second version of the created transaction and the change addresses are not affected.
I believe this behavior was introduced in https://github.com/bitcoin/bitcoin/pull/14582
ACKs for top commit:
achow101:
ACK 14b4921a91920df25b19ff420bfe2bff8c56f71e
Tree-SHA512: a3d56f251ff4b333fc11325f30d05513e34ab0a2eb703fadd0ad98d167ae074493df1a24068298336c6ed2da6b31aa2befa490bc790bbc260ed357c8f2397659
7356292e1d7a44da8a2bd31c02c58d550bf38009 Have zmq reorg test cover mempool txns (Gregory Sanders)
a0f4f9c983e57cc97ecbc56d0177eaf1854c842c Add zmq test for transaction pub during reorg (Gregory Sanders)
2399a0600ca9c4b676fa2f97520b8ecc44642246 Add test case for mempool->block zmq notification (Gregory Sanders)
e70512a83c69bc85e96b08ade725594eda3e230f Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders)
Pull request description:
Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.
Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.
Remaining confusion:
1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics.
2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test)
ACKs for top commit:
laanwj:
code review ACK 7356292e1d7a44da8a2bd31c02c58d550bf38009
promag:
Code review ACK 7356292e1d7a44da8a2bd31c02c58d550bf38009.
Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
3e730bf90aaf53c41ff3a778f6aac15d163d1c0c zmq: Fix due to invalid argument and multiple notifiers (João Barbosa)
Pull request description:
ZMQ initialization is interrupted if any notifier fails, and in that case all notifiers are destroyed. The notifier shutdown assumes that the initialization had occurred. This is not valid when there are multiple notifiers and any except the last fails to initialize.
Can be tested by running test/functional/interface_zmq.py from this branch with bitcoind from master.
Closes#17185.
ACKs for top commit:
laanwj:
Code review ACK 3e730bf90aaf53c41ff3a778f6aac15d163d1c0c, thanks for adding a test
Tree-SHA512: 5da710e97dcbaa94896d019e75162d470f6d381ee07c60e5b3e9db93d11e8f7ca9bf2c509efa4486199e88c96c3e720cc96b4e35b62725d4c7db8e8e9bf6e09d
72ae20fc142457a200278cb2fedc5e32a3766b58 tests: add sync_all to fix race condition in wallet groups test (Karl-Johan Alm)
Pull request description:
This most likely fixes#19749, the intermittent CI issues with wallet_groups.
This fix is also included in #19743.
Top commit has no ACKs.
Tree-SHA512: dd6ef7f89829483e2278191c21fe0912b51fd2187c10a0fa158339c5ab9f22d93b733ae10f17ef25d8b64f44e596e66dba8d7db5c009343472f422ce4cd67d8f
7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9 test: test the implicit avoid partial spends functionality (Karl-Johan Alm)
b82067bf696c53f22536f9ca2e51949c164f6b06 wallet: try -avoidpartialspends mode and use its result if fees are below threshold (Karl-Johan Alm)
Pull request description:
The `-avoidpartialspends` feature is normally disabled, as it may affect the optimal fee for payments. This PR introduces a new parameter `-maxapsfee` (max avoid partial spends fee) which acts on the following values:
* -1: disable partial spend avoidance completely (do not even try it)
* 0: only do partial spend avoidance if fees are the same or better as the regular coin selection
* 1..∞: use APS variant if the absolute fee difference is less than or equal to the max APS fee
For values other than -1, the code will now try partial spend avoidance once, and if that gives a value within the accepted range, it will use that.
Example: -maxapsfee=0.00001000 means the wallet will do regular coin select, APS coin select, and then pick AKS iff the absolute fee difference is <= 1000 satoshi.
Edit: updated this to reflect the fact we are now using a max fee.
ACKs for top commit:
fjahr:
tested ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
achow101:
ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
jonatack:
ACK 7f13dfb58, code review, debug build, verified the test fails with `AssertionError: not(2 == 1)` for the number of vouts when `-maxapsfee=0.0001` is changed to 0, and verified the new logging with an added assertion.
meshcollider:
Code review ACK 7f13dfb587dd6a7a5b7dfbfe689ae0ce818fe5c9
Tree-SHA512: 475929df57f6191bb4e36bfbcad5a280a64bb0ecd8767b76cb2e44e2301235d0eb294a3f2fac5bbf15d35d7ecfba47acb2285feadb883c9ce31c08377e3afb3c
772ea4844c34ad70d02fd0bd6c0945baa8fff85c wallet: Avoid recursive lock in IsTrusted (João Barbosa)
819f10f6718659eeeec13af2ce831df3a0984090 wallet, refactor: Immutable CWalletTx::pwallet (João Barbosa)
Pull request description:
This change moves `CWalletTx::IsTrusted` to `CWallet` in order to have TSAN. So now `CWallet::IsTrusted` requires `cs_wallet` and the recursive lock no longer happens.
Motivated by https://github.com/bitcoin/bitcoin/pull/19289/files#r473308226.
ACKs for top commit:
meshcollider:
utACK 772ea4844c34ad70d02fd0bd6c0945baa8fff85c
hebasto:
ACK 772ea4844c34ad70d02fd0bd6c0945baa8fff85c, reviewed and tested on Linux Mint 20 (x86_64).
Tree-SHA512: 702ffd928b2f42a8b90de398790649a5fd04e1ac3877558da928e94cdeb19134883f06c3a73a6826c11c912facf199173375a70200737e164ccaea1bec515b2a
0cdf2a77ddfa1d53c6fbd830d557a3f20d7fc365 ci: add tsan debug symbols option (Russell Yanofsky)
9a2f12680b3f00a207f1cdd4e0c50a3c7613aefc ci: Add tsan suppression for race in DatabaseBatch (Hennadii Stepanov)
Pull request description:
Since #19325 was merged, the corresponding change in TSan suppression file gets required.
This PR is:
- an analogous to #19226 and #19450, and
- a temporary workaround for CI fail like https://cirrus-ci.com/task/5741795508224000?command=ci#L4993
ACKs for top commit:
MarcoFalke:
ACK 0cdf2a77ddfa1d53c6fbd830d557a3f20d7fc365
Tree-SHA512: 7832f143887c8a0df99dea03e00694621710378fbe923e3592185fcd3658546a590693b513abffc5ab96e9ef76c9c4bff3330eeee69a0c5dbe7574f34c417220
