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
wallet_multisig_descriptor_psbt.py is refactored in this commit. While
behavior doesn't change we do cleanup the way wallets are accessed
throughout the test as this is done a lot for the various signers
and their multisigs. We also get rid of some shallow methods and
instead inline them for improved readability.
descriptors.md is improved to be more explicit about which wallet
(ie the signer or multisig) is required for each step.
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
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
f293c68be0469894c988711559f5528020c0ff71 MOVEONLY: getting mempool conflicts to policy/rbf (glozow)
8d7179633552f58ca0d23305196dcb4249b6dce7 [validation] quit RBF logic earlier and separate loops (glozow)
badb9b11a6f7e1e693cecc8cd5aae55a197d70e2 call SignalsOptInRBF instead of checking all inputs (glozow)
e0df41d7d584b854c2914d4afe7b21e0af3fbf69 [validation] default conflicting fees and size to 0 (glozow)
b001b9f6de7a039a468cf0f9645f3f0a430fa889 MOVEONLY: BIP125 max conflicts limit to policy/rbf.h (glozow)
Pull request description:
See #22675 for motivation, this is one chunk of it. It extracts some BIP125 logic into policy/rbf:
- Defines a constant for specifying the maximum number of mempool entries we'd consider replacing by RBF
- Calls the available `SignalsOptInRBF` function instead of manually iterating through inputs
- Moves the logic for getting the list of conflicting mempool entries to a helper function
- Also does a bit of preparation for future moves - moving declarations around, etc
Also see #22677 for addressing the circular dependency.
ACKs for top commit:
jnewbery:
Code review ACK f293c68be0469894c988711559f5528020c0ff71
theStack:
Code-review ACK f293c68be0469894c988711559f5528020c0ff71 📔
ariard:
ACK f293c68b
Tree-SHA512: a60370994569cfc91d4b2ad5e94542d4855a48927ae8b174880216074e4fa50d4523dd4ee36efdd6edf2bf7adb87a8beff9c3aaaf6dd323b286b287233e63790
faf7e485e901d6c72db5d969b526fa148060a003 Set regtest.BIP65Height = 111 to speed up tests (MarcoFalke)
Pull request description:
No need to waste time by forcing creation of more than 1000 blocks to get the benefits of being able to test BIP 65. Also, reducing the height makes it more likely that (third-party) tests are conforming to BIP 65, which is enforced on mainnet for all new blocks.
ACKs for top commit:
theStack:
re-ACK faf7e485e901d6c72db5d969b526fa148060a003 📍
Zero-1729:
re-ACK faf7e485e901d6c72db5d969b526fa148060a003
kristapsk:
ACK faf7e485e901d6c72db5d969b526fa148060a003
Tree-SHA512: 79a8263e7233838666b9b636b496a8b9eb12398c779f9434677e1d62816732c0a7c7b3e73965be1fb0038d35e05e5a90e665bd74e9610104127dfc4ea38169bf
7720d4f650015272dc7109238230520f71858c6c test: fix failure in feature_nulldummy.py on single-core machines (Sebastian Falbesoner)
646b3885f795c640a2ac979362c509c4a8ee592a test: refactor: use named args for block_submit in feature_nulldummy.py (Sebastian Falbesoner)
Pull request description:
On single-core machines, executing the test `feature_nulldummy.py` results in the following assertion error:
```
...
2021-08-18T15:37:58.805000Z TestFramework (INFO): Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation
2021-08-18T15:37:58.814000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "[...]/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "[...]/test/functional/feature_nulldummy.py", line 107, in run_test
self.block_submit(self.nodes[0], [test4tx], accept=False)
File "[...]/test/functional/feature_nulldummy.py", line 134, in block_submit
assert_equal(None if accept else 'block-validation-failed', node.submitblock(block.serialize().hex()))
File "[...]/test/functional/test_framework/util.py", line 49, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(block-validation-failed == non-mandatory-script-verify-flag (Dummy CHECKMULTISIG argument must be zero))
2021-08-18T15:37:58.866000Z TestFramework (INFO): Stopping nodes
...
```
There are hardly any single-core machines around anymore, but the behaviour can be reproduced on a multi-core machine by patching the function `GetNumCores()` to return 1 on the master branch and running `feature_nulldummy.py`:
```diff
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 30d410381..149b512fc 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -1338,7 +1338,7 @@ bool SetupNetworking()
int GetNumCores()
{
- return std:🧵:hardware_concurrency();
+ return 1;
}
```
As solution, parallel script verification is disabled (`-par=1`) and the exact reject reason is checked, which also increases the precision of the test (the possibility that the block is rejected because of another unintended reason is ruled out). See also related PR #22711 which applies the same approach for the p2p segwit test. The PR also includes a refactoring commit which changes the calls to `self.block_submit()` to use named arguments and removes the default value for parameter `accept` (i.e. explicitely passing `accept=...` is mandatory), with the aim to increase the test readability.
ACKs for top commit:
josibake:
ACK 7720d4f650
Saviour1001:
Tested ACK <code>[7720d4f](7720d4f650)</code>
Tree-SHA512: 8a31ebab3e2ab38e555d7a23139b3324a134a0dedc5b879a2419348ae858323882dbbfcbbf88b68e4f8d7eea8cfe43ee19da1d0d2a36c93ae7878c4980cac31d
