This avoids having to remember to set it whenever mocktime is used with
peer connections. Also, it might help avoiding disconnects when
attaching a debugger to a running test.
The new name describes better what the bool does and also limits the confusion of the three different concepts:
* fBlocksOnly (This bool to skip tx invs)
* -blocksonly (A setting to ignore incoming txs)
* block-relay-only (A connection type in the block-relay-only P2P graph)
fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae Raise InitError when peers.dat is invalid or corrupted (MarcoFalke)
fa4e2ccfd8ae96c381947285bef47cb39474ac89 Inline ReadPeerAddresses (MarcoFalke)
fa5aeec80c6cdca9ca027d80dff3b397911ff2c2 Move LoadAddrman from init to addrdb (MarcoFalke)
Pull request description:
peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:
* The user provided the database, but picked the wrong one.
* A future version of Bitcoin Core wrote the file and it can't be read.
* The file was corrupted by a logic bug in Bitcoin Core.
* The file was corrupted by a disk failure.
ACKs for top commit:
jonatack:
Code review re-ACK fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae per `git range-diff eb1f570 fa59c6d fa55c3` and verified the new tests fail on master, except "Check mocked addrman is valid", as expected
prayank23:
tACK fa55c3dc1b
vasild:
ACK fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae
Tree-SHA512: 78264a78ee570a3c3262cf9c8542b5ffaffa5f52da1eef66c8c381f346989272967cfe1769c573502d9d7d3f7ad68c3ac3b2ec734185d2e4e7595b7122b14196
fa676dbac87919061de9f82bce65e373e8d85bd1 test: pep-8 whitespace (MarcoFalke)
faed284eabb250a07331dfca22bb8f96a95c72ea test: Avoid intermittent test failure in feature_csv_activation.py (MarcoFalke)
Pull request description:
Otherwise there will be disconnects if the test runs longer than the default peertimeout (60s):
```
node0 2021-09-05T20:28:30.973116Z (mocktime: 2021-09-01T07:17:29Z) [net] [net.cpp:1323] [InactivityCheck] socket receive timeout: 393061s peer=0
```
Fix that by skipping `InactivityCheck` via a large `-peertimeout`.
ACKs for top commit:
fanquake:
ACK fa676dbac87919061de9f82bce65e373e8d85bd1
Tree-SHA512: 061c0585a805aa2f8e55c4beedd4b8498a2951f33d60aa3632dda0a284db3a627d14a23dbd57e8a66c69a1612f39418e3a755c8ca97f6ae1105c0d70f0d1a801
c427a5800bb53208d30eeb03a73ab8be879e5f45 doc: Set PYTHONUTF8=1 for functional tests on Windows (Hennadii Stepanov)
Pull request description:
The `PYTHONUTF8` environment variable is defined in [PEP 540](https://www.python.org/dev/peps/pep-0540/), and it is actually used in our CI: 5e3380b9f5/.cirrus.yml (L89)
This PR documents such usage to avoid users' [errors](https://github.com/bitcoin/bitcoin/pull/22922#issuecomment-915511037).
ACKs for top commit:
MarcoFalke:
cr ACK c427a5800bb53208d30eeb03a73ab8be879e5f45
Tree-SHA512: 441b8cecfe47d548cfe403b0e1cd0aef25c1a70ff556434ead1f1e26372919931ac6f208a4ed6fd8dcca46e8709245e4fb06f95259a43c8e1221473ce1ee497b
fa7db1cbf7e8400b625fccd5757f8e1b200796bd [test] checks descendants limtis for second generation Package descendants (ritickgoenka)
Pull request description:
This PR adds a new functional test to test the new descendant limits for packages that were proposed in #21800.
```
+----------------------+
| |
| M1 |
| ^ ^ |
| M2 ^ |
| . ^ |
| . ^ |
| . ^ |
| . ^ |
| M24 ^ |
| ^ |
| P1 |
| ^ |
| P2 |
| |
+----------------------+
```
This test is for checking a transaction to fail its descendant count limits because of a combination of mempool descendants, package direct descendants, and package indirect descendants.
In this test, P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when combined P2 has M1 as an ancestor and M1 exceeds descendant_limits (23 in-mempool descendants + 2 in-package descendants, a total of 26 including itself)
ACKs for top commit:
ryanofsky:
Code review ACK fa7db1cbf7e8400b625fccd5757f8e1b200796bd. Only were suggested changes since last review: simplifying test and dropping P3 transaction as John suggested, and adding assert_equal I suggested
glozow:
ACK fa7db1cbf7e8400b625fccd5757f8e1b200796bd
jnewbery:
ACK fa7db1cbf7
Tree-SHA512: d1eb993550ac8ce31cbe42e17c6522a213ede66970d5d9391f31a116477ab5889fefa6ff2df6ceadd63a28c1be1ad893b0e8e449247e9ade2ca61dc636508d68
e6998838e5548991274ad2bf1697d862905b8837 doc: Add IPv6 address to zmq example (nthumann)
8abe5703a9bb76bc92204a6f69775790e96208fa test: Add IPv6 test to zmq (nthumann)
ded449b726e47f35798ef1c4b1e59123a0dc2b61 zmq: Enable IPv6 on listening socket (nthumann)
Pull request description:
This PR adds support for listening on IPv6 addresses with bitcoinds ZMQ interface, just like the RPC server.
Currently, it is not possible to specify an IPv6 address, as the `ZMQ_IPV6` [socket option](http://api.zeromq.org/master:zmq-setsockopt#toc27) is not set and therefore the ZMQ initialization fails, if one does so. The absence of this option has also been noted [here](https://github.com/bitcoin/bitcoin/issues/15198#issuecomment-617378512).
With this PR one can e.g. set `-zmqpubhashblock=tcp://[::1]:28333` to listen on the IPv6 loopback address.
ACKs for top commit:
laanwj:
Code review ACK e6998838e5548991274ad2bf1697d862905b8837
theStack:
Tested ACK e6998838e5548991274ad2bf1697d862905b8837 🌱
Tree-SHA512: 43c3043d8d5c79794d475926259c1be975b694db4fcc1f7750a9a28e242f0fa1b531735a63ea5777498003aa5834f6243f39742d0f3941f2f37593d0c7890700
fa0b916971e5bc23ad6396831940a2899ca05402 scripted-diff: Use generate* from TestFramework (MarcoFalke)
Pull request description:
This is needed for #22567.
By using the newly added `generate*` member functions of the test framework, it paves the way to make it easier to implicitly call `sync_all` after block generation to avoid intermittent issues.
ACKs for top commit:
jonatack:
ACK fa0b916971e5bc23ad6396831940a2899ca05402
Tree-SHA512: e74a324b60250a87c08847cdfd7b6ce3e1d89b891659fd168f6dd7dc0aa718d0edd28285374a613f462f34f4ef8e12c90ad44fb58721c91b2ea691406ad22c2a
fab0b55cf060c2b14fae5cee13f0a2dcaebde892 addrman: Fix format string in deserialize error (MarcoFalke)
facce4ca44bc206b7656e297a7fa5dfb83a01012 test: Remove useless overwrite (MarcoFalke)
Pull request description:
The format string is evaluated differently on modern compilers (clang 10 and later, as well as gcc 10 and later).
Work around the behaviour change in compilers by pinning the underlying type of the format arguments.
Can be tested by observing a failing test when running against master compiled with clang 10 or gcc 10 (or later).
ACKs for top commit:
jonatack:
ACK fab0b55cf060c2b14fae5cee13f0a2dcaebde892 verified the test fails on master as expected only at line 61 (assertion fixed by the code change); the last two test additions pass as expected
mzumsande:
ACK fab0b55cf060c2b14fae5cee13f0a2dcaebde892
Tree-SHA512: 07462901435107f3bc79098fd7d06446bfe8fe065fffdd35adfcba8f1dd3c499575006557afe7bc74b79d690c5ef7b58e3e031e908161be5529cf237e3b30609
7e698732836121912f179b7c743a72dd6fdffa72 sync: remove DEBUG_LOCKCONTENTION preprocessor directives (Jon Atack)
9b08006bc502e67956d6ab518388fad6397cac8d log, sync: improve lock contention logging and add time duration (Jon Atack)
3f4c6b87f1098436693c4990f2082515ec0ece26 log, timer: add timing macro in usec LOG_TIME_MICROS_WITH_CATEGORY (Jon Atack)
b7a17444e0746c562ae97b26eba431577947b06a log, sync: add LOCK logging category, apply it to lock contention (Jon Atack)
Pull request description:
To enable lock contention logging, `DEBUG_LOCKCONTENTION` has to be defined at compilation. Once built, the logging is not limited to a category and is high frequency, verbose and in all-caps. With these factors combined, it seems likely to be rarely used.
This patch:
- adds a `lock` logging category
- adds a timing macro in microseconds, `LOG_TIME_MICROS_WITH_CATEGORY`
- updates `BCLog::LogMsg()` to omit irrelevant decimals for microseconds and skip unneeded code and math
- improves the lock contention logging, drops the all-caps, and displays the duration in microseconds
- removes the conditional compilation directives
- allows lock contentions to be logged on startup with `-debug=lock` or at run time with `bitcoin-cli logging '["lock"]'`
```
$ bitcoind -signet -debug=lock
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1920 completed (4μs)
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 started
2021-09-01T12:40:01Z LockContention: cs_vNodes, net.cpp:1302 completed (4μs)
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 started
2021-09-01T12:40:02Z LockContention: cs_vNodes, net.cpp:2242 completed (20μs)
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 started
2021-09-01T12:43:04Z LockContention: ::cs_main, validation.cpp:4980 completed (3μs)
$ bitcoin-cli -signet logging
"lock": true,
$ bitcoin-cli -signet logging [] '["lock"]'
"lock": false,
$ bitcoin-cli -signet logging '["lock"]'
"lock": true,
```
I've tested this with Clang 13 and GCC 10.2.1, on Debian, with and without `--enable-debug`.
ACKs for top commit:
hebasto:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72, added a contention duration to the log message since my [previous](https://github.com/bitcoin/bitcoin/pull/22736#pullrequestreview-743764606) review.
theStack:
re-ACK 7e698732836121912f179b7c743a72dd6fdffa72 🔏⏲️
Tree-SHA512: c4b5eb88d3a2c051acaa842b3055ce30efde1f114f61da6e55fcaa27476c1c33a60bc419f7f5ccda532e1bdbe70815222ec2b2b6d9226f29c8e94e598aacfee7
696c76d6604c9c4faddfc4b6684e2788bb577ba6 tests: Add TrimString(...) tests (practicalswift)
4bf18b089e1bb1f3ab513cbdf6674bd1074f4621 Replace use of boost::trim_right with locale-independent TrimString (Ben Woosley)
93551862a18965bcee0c883c54807e8726e2f50f Replace use of boost::trim use with locale-independent TrimString (Ben Woosley)
Pull request description:
This is [#18130 rebased](https://github.com/bitcoin/bitcoin/pull/18130#issuecomment-900158759).
> `TrimString` is an existing alternative.
> Note `TrimString` uses `" \f\n\r\t\v"` as the pattern, which is consistent with the default behavior of `std::isspace`. See: https://en.cppreference.com/w/cpp/string/byte/isspace
ACKs for top commit:
jb55:
utACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
practicalswift:
ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
jonatack:
ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
theStack:
Code-review ACK 696c76d6604c9c4faddfc4b6684e2788bb577ba6
Tree-SHA512: 6a70e3777602dfa65a60353e5c6874eb951e4a806844cd4bdaa4237cad980a4f61ec205defc05a29f9707776835975838f6cc635259c42adfe37ceb02ba9358d
3633b667ffca5a715d9fb27e977515c1e24f600a Use SelectCoinsSRD if it has less waste (Andrew Chow)
8bf789b4b4b26082aea1d91c4d7aa8b01aedfdcf Add SelectCoinsSRD function (Andrew Chow)
2ad3b5d2ad03f781f564ee697ef11e18b2edcea3 tests: wallet_basic lock needed unspents (Andrew Chow)
b77885f13e480304085c654f1b948b20bba63452 tests: wallet_txn explicilty specify inputs (Andrew Chow)
59ba7d2861359f19fa740da9daad1a199409583f tests: rpc_fundrawtx better test for UTXO inclusion with include_unsafe (Andrew Chow)
a165bfbe44b4db3a40b8d1ba8095035367c932a7 tests: rpc_fundrawtx use specific inputs for unavailable change test (Andrew Chow)
df765a484d84702f066e127813fa45a7d440a957 tests: rpc_fundrawtx lock to UTXO types (Andrew Chow)
Pull request description:
To ease in the use of SRD as our fallback mechanism, this PR adds it as a secondary fallback algorithm in addition to the knapsack solver. Since #22009, the solution with the least waste will be chosen. This pattern is continued with SRD simply being another solution whose waste is compared.
ACKs for top commit:
glozow:
reACK 3633b66 via `git range-diff 981b9d1...3633b66`, thanks for taking the suggestions
laanwj:
Concept and code review ACK 3633b667ffca5a715d9fb27e977515c1e24f600a
Tree-SHA512: 895659f553fea2230990136565bdf18b1328de8b0ce47f06b64bb4d69301f6dd68cb38debe5c24fb6de1317b735fc020a987c541f00bbea65229de47e53adf92
fa01f22e6eb7acdead407c3d263c7e344b4257f4 test: Add missing re.escape() to feature_addrman test (MarcoFalke)
Pull request description:
Needed to run the test on windows
ACKs for top commit:
laanwj:
Code review ACK fa01f22e6eb7acdead407c3d263c7e344b4257f4
hebasto:
ACK fa01f22e6eb7acdead407c3d263c7e344b4257f4, passed 2 consequential runs in my [personal CI](https://cirrus-ci.com/task/6522304080379904).
Tree-SHA512: d7ca4fb882cc6693989ddf6fc092db3259a0619cb8f87293c588484b9c62e6755e9fb1bb2c1ab85fcc8f0349d9bc155ba515e16674c0f6f56236e7fbb14655a8
faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a doc: Add 23061 release notes (MarcoFalke)
faff17bbde6dcb1482a6210bc48b3192603a446f Fix (inverse) meaning of -persistmempool (MarcoFalke)
Pull request description:
Passing `-persistmempool` is currently treated as `-nopersistmempool`
ACKs for top commit:
jnewbery:
reACK faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a
hebasto:
ACK faa9c19a4b27e7fabf7c5deae1b4c4ca612ed01a, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: f34a89a07745dabe340eb845b2a348b79c093e9056f7a21c17e1ba2e278177c9b4cf30e8095791fd645a7f90eb34850b2eee0c869b4f6ec02bf749c73b0e52ee
fad02274ba28189f28f5af536b8ea0ecddf9df55 test: Remove Windows workaround in authproxy (MarcoFalke)
Pull request description:
Might no longer be needed after https://github.com/bitcoin/bitcoin/pull/23089
ACKs for top commit:
hebasto:
ACK fad02274ba28189f28f5af536b8ea0ecddf9df55, passed 3 consequential runs on my [personal CI](https://cirrus-ci.com/task/4897456077930496):
Tree-SHA512: 3c408e068ad7590dc0e5922c0b4cd3bf927e22fe624b13b8ab38db848342d310c9c934f358638fd5234c7b5f10f95d3bddc676deb925dcbba54945e2e8229275
5825b34783545f9470d5ab94b87c918980715675 test: avoid non-determinism in asmap-addrman test (Jon Atack)
Pull request description:
This is the same approach as for the addpeeraddress test in `test/functional/rpc_net.py` in commit 869f1368.
The probability of collision when adding an addrman entry is expected to be 1/2^16 = 1/65536 for an address from a different /16. This change hopes to avoid these collisions by adding 1 tried entry before adding 1 new table one, instead of 2 tried entries followed by 2 new entries, which appears to have caused a collision in the CI.
To verify the regression test still fails when expected:
- `git checkout 181a120 && git cherry-pick ef242f5`
- recompile bitcoind
- git checkout this branch and run `test/functional/feature_asmap.py`. Expected output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. !=
```
Closes#23078.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
ACKs for top commit:
mzumsande:
Re-ACK 5825b34783545f9470d5ab94b87c918980715675
Tree-SHA512: eb6624a196fa5617c84aad860c8f91e8a8f60fc9fe2d7168facc298ee38db5e93b7e988e11c2ac1b399dc2c6b8fad360fb10bdbf10e598a1878300388475a200
This is the same approach as for the addpeeraddress test in
`test/functional/rpc_net.py` in commit 869f1368.
The probability of collision when adding an addrman entry is
expected to be 1/2^16 = 1/65536 for an address from a different /16.
This change hopes to avoid these collisions by adding 1 tried entry
before adding 1 new table one, instead of 2 tried entries followed
by 2 new entries, which appears to have caused a collision in the CI.
To verify the regression test stills fails when expected:
- git checkout 181a120 && git cherry-pick ef242f5
- recompile bitcoind
- git checkout this branch and run test/functional/feature_asmap.py. Expected output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. !=
```
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
Instead of relying on coin selection to deterministically choose
the correct inputs to use, just specify them explicitly and use
the raw transaction RPCs.
Don't assume that specific inputs are going to be used when they aren't
specified explicitly.
Also fixes a bug in the include_unsafe test where after the inputs
confirm, include_unsafe should be set to False rather than True.
For the test that checks that there is no error when change is
unavailable but change is also not needed, use specific UTXOs so that
SRD does not cause this to fail when it chooses random inputs.
For some of the tests within rpc_fundrawtx, there is the expectation
that two independent calls to coin selection RPCs will use the same type
of UTXO. This is not necessarily guaranteed, so to make sure it is, use
lockunspent prior to those tests.
b11a195ef450bd138aa03204a5e74fdd3ddced26 refactor: Detach wallet transaction methods (followup for move-only) (Russell Yanofsky)
Pull request description:
This makes `CWallet` and `CWalletTx` methods in `spend.cpp` and `receive.cpp` files into standalone functions.
It's a followup to [#21207 MOVEONLY: CWallet transaction code out of wallet.cpp/.h](https://github.com/bitcoin/bitcoin/pull/21207), which moved code from `wallet.cpp` to new `spend.cpp` and `receive.cpp` files.
There are no changes in behavior. This is just making methods into functions and removing circular dependencies created by #21207. There are no comment or documentation changes, either. Removed comments from `transaction.h` are just migrated to `spend.h`, `receive.h`, and `wallet.h`.
---
This commit was split off from #21206 so there are a few earlier review comments there
ACKs for top commit:
achow101:
ACK b11a195ef450bd138aa03204a5e74fdd3ddced26
Sjors:
utACK b11a195ef450bd138aa03204a5e74fdd3ddced26
meshcollider:
light ACK b11a195ef450bd138aa03204a5e74fdd3ddced26
Tree-SHA512: 75ce818d3f03b728b14b12e2d21bd20b7be73978601989cb37ff98254393300d1bb7823281449cd3d9e40756d67d42bd9a46bbdafd2e8baa95aaf2cb1c84549f
fa2e9de59f189fe37c3eeb63d79e09983e40a993 test: Check that non-signaling BIP125 tx can be replaced via parent (MarcoFalke)
Pull request description:
While `optout_child_tx` in the `test_no_inherited_signaling` test is reported as "bip125-replaceable", it is not *directly* replaceable. For example by bumping the fee of `optout_child_tx`. However, it is still replaceable *indirectly* via it's BIP-125 signalling parent.
Clarify this by extending the test.
ACKs for top commit:
mjdietzx:
Tested ACK fa2e9de59f189fe37c3eeb63d79e09983e40a993
josibake:
ACK fa2e9de59f
Tree-SHA512: b3608beae743dcb6152df4d2cfe1c0af6b4404ba3837f73e1d1431bd7c637f0c7fab0379aaab2218d5cd63e71070a079c0595ec031056058e8d3c933c2bae0a9
ab9c34237ab7b056394e0bd1f7cb131ffd95754c release: remove gitian (fanquake)
Pull request description:
Note that this doesn't yet touch any glibc back compat related code.
ACKs for top commit:
laanwj:
Code review ACK ab9c34237ab7b056394e0bd1f7cb131ffd95754c
Tree-SHA512: 8e2fe3ec1097f54bb11ab9136b43818d90eab5dbb0a663ad6a552966ada4bdb49cc12ff4e66f0ec0ec5400bda5c81f3a3ce70a9ebb6fe1e0db612da9f00a51a7
387355bb9482a09c1fc9b137bea56745a93b7dfd test, refactor: rpc_rawtransaction PEP8 (Jon Atack)
7d5cec2e498dc059ff1d74a2b60764db45923264 refactor: separate the rpc_rawtransaction tests into functions (Jon Atack)
409779df95f886b08dbf6d44219e2fbeb3405a43 move-only: regroup similar rpc_rawtransaction tests together (Jon Atack)
d861040dd24a321e1ceec1f07c7bb80d59779081 test: remove no longer needed (ASCII art) comments (Jon Atack)
14398b30d6242db14670b3286f988b2badda83fb test: add and harmonize getrawtransaction logging (Jon Atack)
85d8869cf89fedf243748e3e15b3ed39de1b0385 test: run 2nd getrawtransaction section with/without -txindex (Jon Atack)
00977407732969593800d15de39abbb7e0250abc refactor: txid to constant in rpc_rawtransaction to isolate tests (Jon Atack)
8c19d1329f1f28000ca32d826cecf04680c6be69 refactor: dedup/reorg createrawtransaction sequence number tests (Jon Atack)
7f073594c9f5b518dc1fb66dfb0189e8803e3545 Test src/node/transaction::GetTransaction() without -txindex (Jon Atack)
Pull request description:
Following up on https://github.com/bitcoin/bitcoin/pull/22383#pullrequestreview-698583510, this pull adds missing `src/node/transaction::GetTransaction()` test coverage for combinations of `-txindex` and `blockhash` and does some refactoring of the test file.
ACKs for top commit:
mjdietzx:
reACK 387355bb9482a09c1fc9b137bea56745a93b7dfd
josibake:
reACK 387355bb94
MarcoFalke:
Approach ACK 387355bb9482a09c1fc9b137bea56745a93b7dfd 🔆
Tree-SHA512: b47c4ff87d69c61434e5729c954b338bc13744eddaba0879ca9f5f42243ba2cb4640d94c5f74de9f2735a8bf5e66b3d1c3bd3b7c26cd7324da7d3270ce87c6fd
Followup to commit "MOVEONLY: CWallet transaction code out of
wallet.cpp/.h" that detaches and renames some CWalletTx methods, making
into them into standalone functions or CWallet methods instead.
There are no changes in behavior and no code changes that aren't purely
mechanical. It just gives spend and receive functions more consistent
names and removes the circular dependencies added by the earlier
MOVEONLY commit.
There are also no comment or documentation changes. Removed comments
from transaction.h are just migrated to spend.h, receive.h, and
wallet.h.
86beee05795216738f51fa744539336503c26fd9 Use waste metric for deciding which selection to use (Andrew Chow)
b3df0caf7c291a316298e54e73426c765e61c129 tests: Test GetSelectionWaste (Andrew Chow)
4f5ad43b1e05cd7b403f87aae4c4d42e5aea810b Add waste metric calculation function (Andrew Chow)
935b3ddf72aa390087684e03166c707f5b173434 scripted-diff: tests: Use KnapsackSolver directly (Andrew Chow)
6a023a6f904efe38dacd662d919aba74f066b1dc tests: Add KnapsackGroupOutputs helper function (Andrew Chow)
d5069fc1aa7d335f3043227f843cbb9d8ba1507b tests: Use SelectCoinsBnB directly instead of AttemptSelection (Andrew Chow)
54de7b47463d98f860167d4e0b7e4ebb3926b59c Allow the long term feerate to be configured, default of 10 sat/vb (Andrew Chow)
Pull request description:
Branch and Bound introduced a metric that we call waste. This metric is used as part of bounding the search tree, but it can be generalized to all coin selection solutions, including those with change. As such, this PR introduces the waste metric at a higher level so that we can run both of our coin selection algorithms (BnB and KnapsackSolver) and choose the one which has the least waste. In the event that both find a solution with the same change, we choose the one that spends more inputs.
Also this PR sets the long term feerate to 10 sat/vb rather than using the 1008 block estimate. This allows the long term feerate to be the feerate that we switch between consolidating and optimizing for fees. This also removes a bug where the long term feerate would incorrectly be set to the fallback fee. While this doesn't matter prior to this PR, it does have an effect following this. The long term feerate can be configured by the user through a new `-consolidatefeerate` option.
ACKs for top commit:
Xekyo:
reACK 86beee0 via git range-diff fe47558...86beee0
meshcollider:
re-utACK 86beee05795216738f51fa744539336503c26fd9
Tree-SHA512: 54b154b346538eca68ae2a3b83a033b495c1605c14f842bfc43ded2256b110983ce674c647fe753cf0305b1b178403d8d60d6d4203c7a712bec784be52e90d42
