65aaf9495d refactor: move `update_*` structs from txmempool.h to .cpp file (Sebastian Falbesoner)
9947ce6262 refactor: use const reference for parents in `CTxMemPool::UpdateAncestorsOf` (Sebastian Falbesoner)
Pull request description:
These helpers are exclusively used in txmempool.cpp, hence they should also be moved there. The PR also contains a commit which fixes const-correctness for parents in `CTxMemPool::UpdateAncestorsOf` and declares them as reference to avoid a copy.
ACKs for top commit:
promag:
Code review ACK 65aaf9495d. Verified move-only commit locally.
Tree-SHA512: 7ce29f3ba0e68b5355001f27725b00f6d54cc993015356eb40b61b8cdd17db49b980f4c3d798c8e0c940d245dc3a72c474bb9ff3c0ee971ead450786076812c2
6ae9f1cf96 Disable lock contention logging in checkqueue_tests (Jon Atack)
Pull request description:
This patch disables lock contention logging in the checkqueue_tests as some of these tests are designed to be heavily contested to trigger race conditions or other issues. This created very large log files when run with DEBUG_LOCKCONTENTION defined (up to v22) or with lock logging enabled by default in current master.
Examples running the following command:
```
$ ./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_Random -- DEBUG_LOG_OUT > testlog.txt
-rw-r--r-- 87042178 Oct 8 12:41 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run1.txt
-rw-r--r-- 73879896 Oct 8 12:42 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run2.txt
-rw-r--r-- 65150518 Oct 8 12:51 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run1.txt
-rw-r--r-- 65774554 Oct 8 12:52 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run2.txt
-rw-r--r-- 73493309 Oct 8 13:00 testlog-current-master-at-991753e-run1.txt
-rw-r--r-- 65616977 Oct 8 13:01 testlog-current-master-at-991753e-run2.txt
-rw-r--r-- 5093 Oct 8 13:04 testlog-with-this-commit-run1.txt
-rw-r--r-- 5093 Oct 8 13:05 testlog-with-this-commit-run2.txt
```
Resolves#23167.
ACKs for top commit:
vasild:
ACK 6ae9f1cf96
Tree-SHA512: b16812ed60c58a1cf40c04ebeca9197ac076b2415f71673ac7bb5b7960a1ff80ba2c909345ad221c7689b0562d17f63a32a629f5d6dbcf0e57130bf5760388c1
As defined in BIP340, a verification step should be executed after
`secp256k1_schnorrsig_sign` to ensure that a potentially corrupted
signature isn't used; using corrupted signatures could reveal
information about the private key used. This applies to ECSDA as
well.
Additionally clears schnorr signature if signing failed.
f3e451bebf [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev)
8c96008ab1 [net] Don't return an optional from TransportDeserializer::GetMessage() (Troy Giorshev)
Pull request description:
Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This
throws an error if COMMAND_OTHER doesn't exist, which should never
happen. `find()` instead just accessed the last element, which could make
debugging more difficult.
Resolves review comments from PR19107:
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497
ACKs for top commit:
theStack:
Code-review ACK f3e451bebf
ryanofsky:
Code review ACK f3e451bebf. Changes since last review in https://github.com/bitcoin/bitcoin/pull/20364#pullrequestreview-534369904 were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit.
Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
68018e4c3e test: Avoid excessive locking of `cs_wallet` (Hennadii Stepanov)
7986faf2e0 test: Fix segfault in the psbt_wallet_tests/psbt_updater_test (Hennadii Stepanov)
Pull request description:
The dcd6eeb64a commit (bitcoin/bitcoin#23288) introduced an intermittent failure in the `psbt_wallet_tests/psbt_updater_test` unit test. See bitcoin/bitcoin#23368.
The test failure can be easily made reproducible with the following patch:
```diff
--- a/src/scheduler.cpp
+++ b/src/scheduler.cpp
@@ -57,6 +57,8 @@ void CScheduler::serviceQueue()
Function f = taskQueue.begin()->second;
taskQueue.erase(taskQueue.begin());
+ UninterruptibleSleep(100ms);
+
{
// Unlock before calling f, so it can reschedule itself or another task
// without deadlocking:
```
This PR implements an idea which was mentioned in the [comment](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-953796339):
> Yes, as I said before this looks like a race where the wallet is deleted before stopping the scheduler: [#23368 (comment)](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-952808824)
>
> IIRC, the order should be:
>
> * stop scheduler
>
> * delete wallet
>
> * delete scheduler
The second commit introduces a refactoring with no behavior change.
Fixesbitcoin/bitcoin#23368.
ACKs for top commit:
mjdietzx:
Code review ACK 68018e4c3e
Tree-SHA512: d9103f6252aab807453628159dec243bc543a2595eecaa04ec761dca3c2370085592c55d6f50967d69a4ac6e8b5827eec30dd9b025132c99b0bb9aa5911ad915
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
c5d7e34bd9 scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags (Russell Yanofsky)
b8c069b7a9 refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage (Russell Yanofsky)
26a50ab322 refactor: Split InterpretOption into Interpret{Key,Value} functions (Russell Yanofsky)
Pull request description:
This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.
---
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).
An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed.
Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
None of the changes affect behavior in any way.
ACKs for top commit:
ajtowns:
utACK c5d7e34bd9
promag:
Code review ACK c5d7e34bd9, which as the new argument `-legacy`.
Tree-SHA512: cad0e06361e8cc584eb07b0a1f8b469e3beea18abb458c4e43d9d16e9f301b12ebf1d1d426a407fbd96f99724ad6c0eae5be05c713881da7c55e0e08044674eb
61ec0539b2 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7b7b [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6d68 [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e833 [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)
Pull request description:
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
ACKs for top commit:
naumenkogs:
ACK 61ec0539b2
mzumsande:
ACK 61ec0539b2
shaavan:
ACK 61ec0539b2
Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
9ba7c44265 refactor: get wallet path relative to wallet_dir (Michael Dietz)
Pull request description:
Now that boost has been updated > 1.60 (see #22320), we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`, removing a TODO.
Test coverage comes from `test/functional/wallet_multiwallet.py`
I first tried this in #20265 which was my first attempted PR, and funny enough exactly 1 year later I'm opening this one to hopefully finally close this.
ACKs for top commit:
ryanofsky:
Code review ACK 9ba7c44265. Basically this same code change is made in #20744 commit b70c84348ac7a8e427a1183f894c73e52c734529, so this PR helps simplify that one
lsilva01:
Code Review ACK 9ba7c44
Tree-SHA512: 6ccb91a18bcb52c3ae0c789a94a18fb5be7db7769fd1121552d63f259fbd32b50c3dcf169cec0b02f978321db3bc60eb4b881b8327e9764f32e700236e0d8a35
Now that boost has been updated > 1.60, we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`
d891ae7681 Introduce new V4 format addrman (Pieter Wuille)
Pull request description:
#23306 effectively changed the on-disk format in an incompatible way: old deserializers cannot deal with multiple entries for the same IP.
Introduce a `V4_MULTIPORT` format, and increment the compatibility base, so that old versions correctly recognize it as an incompatible future version, rather than corruption.
ACKs for top commit:
naumenkogs:
ACK d891ae7681
ajtowns:
utACK d891ae7681
vasild:
ACK d891ae7681
Tree-SHA512: de2153beb59152504ee0656dd0cc0b879b09136eb07e3ce0426d2fea778adfabacebbce5cf1a9a65dc99ad4e99cda42ab26743fe672fb82a9fbfec49c4cccb4d
d2c4904ef7 test: MiniWallet: more deterministic coin selection for coinbase UTXOs (oldest first) (Sebastian Falbesoner)
Pull request description:
The coin selection strategy for MiniWallet is quite straight-forward: simply pick a single UTXO with the largest value:
ab25ef8c7f/test/functional/test_framework/wallet.py (L173-L174)
If there are several candidates with the same value, however, it is not clear which one is taken. This can be particularly problematic for coinbase outputs with fixed block subsidy, since spending could lead to a `bad-txns-premature-spend-of-coinbase` reject if an UTXO from a too-recent block is picked. Introduce block height as second criteria (saved in `self._utxos` in the methods `generate(...)` and `rescan_utxos(...)`), in order to avoid potential issues with coinbases that are not matured yet. If there is a tie between coinbase UTXOs and non-coinbase UTXOs (the latter are added via `scan_tx(...)`), prefer the non-coinbase UTXOs, since those don't need to mature.
The issue came up while refactoring the test rpc_blockchain.py, see https://github.com/bitcoin/bitcoin/pull/23371#discussion_r737401936 (PR #23371).
ACKs for top commit:
MarcoFalke:
review ACK d2c4904ef7
shaavan:
ACK d2c4904ef7
Tree-SHA512: 15d67b42fb8b77fd53022ea2ab8a6ed2b615567f3ce73bab16c06bfcb687c1a04dcb0360d0c2287c526b604cd3ac5eef7b14ce46fc31e23047ce1a3290027306
2600db6c36 test: fix misleading fee unit in mempool_limit.py (Sebastian Falbesoner)
Pull request description:
The PR is a follow-up to #22543. The helper `send_large_txs` in its current interface has a fee_rate parameter, implying that it would create a transaction with exactly that rate. Unfortunately, this fee rate is only passed to MiniWallet's `create_self_transfer` method, which can't know that we append several tx outputs after, increasing the tx's vsize and decreasing it's fee rate accordingly.
In our case, the fee rate is off by several orders of magnitude, as the tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the value passed to this function is neither really a fee rate nor an absolute fee, but something in-between, which is very confusing. It was suggested to simply in-line this helper as it's currently only used in this single test (https://github.com/bitcoin/bitcoin/pull/22543#discussion_r701685136, https://github.com/bitcoin/bitcoin/pull/22543#issuecomment-918986896), but I could imagine that this helper may also become useful for other tests and may be moved to a library (e.g. wallet.py) in the future.
Clarify the interface by passing an absolute fee that is deducted in the end (and also verified, via testmempoolaccept) and also describe how we come up with the value passed. On master, the comment says that the fee rate needs to increased "massively"; this word is also removed because the fee rate only needs to be higher for the test to succeed.
ACKs for top commit:
stratospher:
ACK 2600db6.
Tree-SHA512: 0bfacc3fa87603970d86c1d0186e51511f6c20c64b0559e19e7e12a68647f79dcb4f436000dee718fd832ce6a68e3bbacacb29145e0287811f1cb03d2f316843
54011e7aa2 refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa2
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
This is not only cleaner but also helps make sure we are always using
the virtual size measure that includes the sigop weight heuristic (which
is the vsize the mempool would return).
This bool was originally part of Workspace and was removed in #22539
when it was no longer needed in Finalize(). Re-introducing it because,
once again, multiple functions will need to know whether we're doing an
RBF. Member of MemPoolAccept so that we can use this to inform package
RBF in the future.
No change in behavior.
ATMPArgs can continue to have granular rules like switching BIP125
on/off while we create an interface for the different sets of rules for
single transactions vs multiple-testmempoolaccept vs package validation.
This is a cleaner interface than manually constructing the args, which
makes it easy to mix up ordering, use the wrong default, etc. It also
means we don't need to edit ATMP/single transaction validation code
every time we update ATMPArgs for package validation.
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).
The coin selection strategy for MiniWallet is quite straight-forward: simply
pick a single UTXO with the largest value:
self._utxos = sorted(self._utxos, key=lambda k: k['value'])
utxo_to_spend = utxo_to_spend or self._utxos.pop()
If there are several candidates with the same value, however, it is not clear
which one is taken. This can be particularly problematic for coinbase outputs
with fixed block subsidy, since spending could lead to a
'bad-txns-premature-spend-of-coinbase' reject if an UTXO from a too-recent
block is picked. Introduce block height as second criteria (saved in
self._utxos in the methods generate(...) and rescan_utxos(...)), in order to
avoid potential issues with coinbases that are not matured yet.
4718897ce3 test: add script_util helper for creating bare multisig scripts (Sebastian Falbesoner)
Pull request description:
This PR is a follow-up to #22363 and #23118 and introduces a helper `keys_to_multisig_script` for creating bare multisig outputs in the form of
```
OP_K PubKey1 PubKey2 ... PubKeyN OP_N OP_CHECKMULTISIG
```
The function takes a list of pubkeys (both hex- and byte-strings are accepted due to the `script_util.check_key` helper being used internally) and optionally a threshold _k_. If no threshold is passed, a n-of-n multisig output is created, with _n_ being the number of passed pubkeys.
ACKs for top commit:
shaavan:
utACK 4718897ce3
rajarshimaitra:
tACK 4718897ce3
Tree-SHA512: b452d8a75b0d17316b66ac4ed4c6893fe59c7c417719931d4cd3955161f59afca43503cd09b83a35b5a252a122eb3f0fbb9da9f0e7c944cf8da572a02219ed9d
d5f985e51f multiprocess: Add new bitcoin-gui, bitcoin-qt, bitcoin-wallet init implementations (Russell Yanofsky)
Pull request description:
Add separate `interfaces::Init` subclasses for `bitcoin-wallet`, `bitcoin-gui`, and `bitcoin-qt` binaries instead of sharing `bitcoind` and `bitcoin-node` init subclasses in different binaries. After this, the new init subclasses can be customized in #10102, so node and wallet code is dropped from the `bitcoin-gui` binary and wallet code is dropped from into the `bitcoin-node` binary.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
lsilva01:
reACK d5f985e
hebasto:
re-ACK d5f985e51f, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/23006#pullrequestreview-787537444) review.
Tree-SHA512: 6784210bd9ce3a6fbc66852680d0e9bc513c072dc538aeb7f48cb6a41580d3f567ccef04f975ee767a714a4b05d4d87273e94a79abda1b9c25d5ac4bbe752006
92617b7a75 effectively changed the
on-disk format in an incompatible way: old deserializers cannot
deal with multiple entries for the same IP.
Introduce a V4_MULTIPORT format, and increment the compatibility base,
so that old versions correctly recognize it as an incompatible future
version.
This commit does not change behavior in any way. See previous commit for
complete rationale, but these flags are being disabled because they
aren't implemented and will otherwise break backwards compatibility when
they are implemented.
-BEGIN VERIFY SCRIPT-
sed -i 's:\(ALLOW_.*\) \(//!< unimplemented\):// \1\2:' src/util/system.h
sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp
git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)' | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g'
git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g'
-END VERIFY SCRIPT-
92617b7a75 Make AddrMan support multiple ports per IP (Pieter Wuille)
Pull request description:
For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.
The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in https://github.com/bitcoin/bitcoin/issues/5150#issuecomment-853888909 e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).
And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.
One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket _already_ uses the port number, the only change required is making all addrman lookup be (IP,port) (aka `CService`) based, rather than IP (aka `CNetAddr`) based.
This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.
ACKs for top commit:
jnewbery:
Code review ACK 92617b7a75
naumenkogs:
ACK 92617b7a75
ajtowns:
ACK 92617b7a75
vasild:
ACK 92617b7a75
Tree-SHA512: 9eef06ce97a8b54a3f05fb8acf6941f253a9a5e0be8ce383dd05c44bb567cea243b74ee5667178e7497f6df2db93adab97ac66edbc37c883fd8ec840ee69a33f
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation,
so current uses of these flags are misleading and will also break
backwards compatibility whenever these flags are implemented in a future
PR (draft PR is #16545).
An additional complication is that while these flags don't do any real
settings validation, they do affect whether setting negation syntax is
allowed.
Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are
implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is
done in two commits, with this commit adding the DISALLOW_NEGATION flag,
and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
e9ade032f3 tests: Add feature_segwit.py --descriptors to test_runner.py (Andrew Chow)
ae6cbcc909 tests: restrict feature_segwit legacy wallet import tests (Andrew Chow)
1d13c44a4c tests: Use descriptors for feature_segwit multisig setup (Andrew Chow)
Pull request description:
`feature_segwit.py` has tests for some legacy wallet behavior, but otherwise does not really need the legacy wallet. Those parts now require `--legacy-wallet` to be provided (as with other legacy wallet tests). Other parts of the test are changed to work with descriptor wallets as well.
ACKs for top commit:
lsilva01:
tACK e9ade03
Tree-SHA512: 4ae76a4d2a8d318e7f8ad4c984748e3cdd67ed00359fcd798a08492ed9399b1d01be88c9ebea3d6c996fbe190cf41708a15c98f088f0cb5c47d6d00fff258944
082c5bf099 [refactor] pass coinsview and height to check() (glozow)
ed6115f1ea [mempool] simplify some check() logic (glozow)
9e8d7ad5d9 [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d18916af MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec26a [mempool] remove now-unnecessary code (glozow)
54c6f3c1da [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f65e [bench] Benchmark CTxMemPool::check() (glozow)
cb1407196f [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)
Pull request description:
Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.
This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.
ACKs for top commit:
jnewbery:
reACK 082c5bf099
GeneFerneau:
tACK [082c5bf](082c5bf099)
rajarshimaitra:
tACK 082c5bf099
theStack:
Code-review ACK 082c5bf099
Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
45827fd718 test: check for block reject reasons in p2p_segwit.py [2/2] (Sebastian Falbesoner)
4eb532ff8b test: check for block reject reasons in p2p_segwit.py [1/2] (Sebastian Falbesoner)
b1488c4dce test: fix reference to block processing test in p2p_segwit.py (Sebastian Falbesoner)
Pull request description:
In the test `p2p_segwit.py`, there are many instances where we send a segwit block to a node with the expectation that it is rejected. For this purpose, the helper function `test_witness_block` exists which allows also to check for a specific reject `reason` that is asserted in the debug log:
502d22ceed/test/functional/p2p_segwit.py (L119-L120)
This PR aims to increase the precision of the tests by adding the expected reject reasons to all `test_witness_block` call instances (found via `grep`ing after `test_witness_block(.*accepted=False`). For some blocks that are rejected due to failed script verification, the exact reason is only shown in the debug log if parallel script verification is disabled. For this reason, the addition of the reasons is split up in two commits:
* first, all instances are tackled that are not subject to script verification, i.e. it doesn't matter whether parallel script verification is enabled or not (e.g. `bad-blk-weight`, `bad-witness-merkle-match`...)
* then, we explicitely set `-par=1` to only use one script thread, and add the remaining reasons (`non-mandatory-script-verify-flag` with the more specific reason in parantheses)
ACKs for top commit:
stratospher:
tested ACK 45827fd.
Tree-SHA512: 00f31867f11d48b38a42b1e79a1303bda1c797ccf3d8c73e6107d70b062604d51ee2a3f2251e7f068dfafdaf09469d27dfee438d9bc9ebaef7febc4b6ef90a95
a52f1d1340 walletdb: Use SQLiteDatabase for mock wallet databases (Andrew Chow)
a78c229808 tests: Place into mapWallet in coinselector_tests (Andrew Chow)
Pull request description:
#23288 changed coinselector_tests to use `DescriptorScriptPubKeyMan`, but it also ended up significantly slowing down the test, from 4 seconds to over 1 minute. It appears that the source of this slow down is with `CWallet::AddToWallet`, and primarily due to writing data to the mock wallet database. Because the only thing that is actually needed is for the created transaction to be placed into `CWallet::mapWallet`, this PR removes the call to `AddToWallet` and just places the transaction into `mapWallet` directly. This reduces the test time to 5 seconds.
To speed things up further, `CreateMockWalletDatabase` is changed to make a `SQLiteDatabase` instead of a `BerkeleyDatabase`. This is safe because there are no tests that require a specific mock database type.
ACKs for top commit:
brunoerg:
tACK a52f1d1340
lsilva01:
tACK a52f1d1. Performed 74.36% better on Ubuntu 20.04 (VM, 12 MB, 8vCPU).
glozow:
utACK a52f1d1340
Tree-SHA512: da77936bfd2e816d2e71703567b9389d0ee79f3a4a690802ffe3469df5bed371b296cb822b897f625654dab9436d91fd6bc02364a518a47d746e487d70a72595
While const shared_ptr<X> gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr<const X> gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr<X> into shared_ptr<const X>, but not the other way around. We do this for two reasons: because it makes the code safer to guarantee the wallet is not modified, and because it further dispells the misconception that const shared_ptr<X> gives immutability to X.
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
f778845d97 ci: Add vcpkg tools cache (Hennadii Stepanov)
Pull request description:
On master (02feae54a5) vcpkg downloads some tools that are used internally:
```
...
A suitable version of cmake was not found (required v3.20.0). Downloading portable cmake v3.20.0...
Downloading cmake...
https://github.com/Kitware/CMake/releases/download/v3.20.2/cmake-3.20.2-windows-i386.zip -> C:\Users\ContainerAdministrator\AppData\Local\Temp\vcpkg\downloads\cmake-3.20.2-windows-i386.zip
Extracting cmake...
A suitable version of 7zip was not found (required v18.1.0). Downloading portable 7zip v18.1.0...
Downloading 7zip...
https://www.nuget.org/api/v2/package/7-Zip.CommandLine/18.1.0 -> C:\Users\ContainerAdministrator\AppData\Local\Temp\vcpkg\downloads\7-zip.commandline.18.1.0.nupkg
Extracting 7zip...
A suitable version of nuget was not found (required v5.5.0). Downloading portable nuget v5.5.0...
Downloading nuget...
https://dist.nuget.org/win-x86-commandline/v5.5.1/nuget.exe -> C:\Users\ContainerAdministrator\AppData\Local\Temp\vcpkg\downloads\22ea847d-nuget.exe
Detecting compiler hash for triplet x64-windows-static...
A suitable version of powershell-core was not found (required v7.1.0). Downloading portable powershell-core v7.1.0...
Downloading powershell-core...
https://github.com/PowerShell/PowerShell/releases/download/v7.1.3/PowerShell-7.1.3-win-x86.zip -> C:\Users\ContainerAdministrator\AppData\Local\Temp\vcpkg\downloads\PowerShell-7.1.3-win-x86.zip
Extracting powershell-core...
...
```
If any of downloads above fails the whole CI task fails (see #23107). The most recent failure examples in the master branch:
- c001da306b, [log](https://api.cirrus-ci.com/v1/task/5182966176415744/logs/build.log)
- 12ff8993bc, [log](https://api.cirrus-ci.com/v1/task/5367684129882112/logs/build.log)
This PR adds vcpkg tools cache.
Closes#23107.
---
Note for reviewers. Some patches from the initial PR were split into separated PRs: #23310 and #23329. Therefore, a discussion here could be outdated or irrelevant until the recent [push](https://github.com/bitcoin/bitcoin/pull/23215#issuecomment-949556531).
ACKs for top commit:
sipsorcery:
ACK f778845d97.
Tree-SHA512: 201f4e4d38c00cbec6aaeca4f3e1b60f1c65289fb68b82cead47f37f6af5ac42dd539cf8ed3566f5bd9afe4a7762d42bbabb316d58f47352d4e7bfc2214806fe
Default to SQLiteDatabase instead of BerkeleyDatabase for
CreateDummyWalletDatabase. Most tests already use descriptor wallets and
the mock db doesn't really matter for tests. The tests where it does
matter will make the db directly.
Instead of using AddToWallet so that making a COutput will work,
directly add the transaction into wallet.mapWallet. This bypasses many
checks that AddToWallet will do which are pointless and just slow down
this test.
58765a450c qt: Use only Qt translation primitives in GUI code (W. J. van der Laan)
Pull request description:
Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up.
Replaces bitcoin/bitcoin#22764
Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`:
- "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core`
- "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen`
ACKs for top commit:
hebasto:
ACK 58765a450c
Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
