be8d0dba15 test: refactor: save MiniWallet mode explicitly (Sebastian Falbesoner)
Pull request description:
Rather than abusing the member variables `self._priv_key` and `self._address` to determine the MiniWallet mode, save it explicitly (`self._mode`) in the constructor and use that instead to increase the readability and maintainability of the code.
ACKs for top commit:
MarcoFalke:
review ACK be8d0dba15🔳
Tree-SHA512: 882c635e39c89911d995917a0603395158ee86dc46b26a49815756db67c61a7aa35059eddf1dc6f2933a77169941426b98bd463e60e39515a596b1b69edf89df
Rather than abusing the member variables self._priv_key and
self._address to determine the MiniWallet mode, save it explicitly
instead in the constructor to increase the readability and
maintainability of the code.
While glibc 2.25 and newer *can* be built with stack-smashing-protection
enabled, it isn't used by default, and still isn't, as of glibc 2.35,
so I can't see a reason to explicitly disable it.
I'd also like to move in the direction of enabling, by default,
hardening options for the toolchains we build, so removing the explicit
disabling is a step in that direction.
Will be following up with some changes based on this PR.
40566e21c0 If -prune=0 is set, Uncheck Prune on Intro page (Jadi)
Pull request description:
If the bitcoin-qt is started with -prune=0 arg, On the Intro page,
the Prune Checkbox will be unchecked too, to prevent confusions.
refs: https://github.com/bitcoin/bitcoin/issues/25052
ACKs for top commit:
hebasto:
re-ACK 40566e21c0
Tree-SHA512: d5e0b76a7d20ae806e61a416fd907650f15a744a5823d0f8b57a634cb099bb135199e69a787bd54ecde2cf84e95633f40ff407a722350f337b27de395a6e0f78
d338712886 scripted-diff: rename fAllowOtherInputs -> m_allow_other_inputs (furszy)
8dea74a8ff refactor: use GetWalletTx in SelectCoins instead of access mapWallet (furszy)
b4e2d4d4ee wallet: move "use-only coinControl inputs" below the selected inputs lookup (furszy)
25749f1df7 wallet: unify “allow/block other inputs“ concept (furszy)
Pull request description:
Seeking to make the `CoinControl` options less confusing/redundant.
It should have no functional changes.
The too long to read technical description; remove `m_add_inputs`, we can use the already existent `fAllowOtherInputs` flag.
In #16377 the `CoinControl` flag ‘m_add_inputs’ was added to tell the coin filtering and selection process two things:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: Search the wtxs-outputs and append all the `CoinControl` internal and external selected outpoints to the selection result (skipping all the available output checks). Nothing else.
Meanwhile, in `CoinControl` we already have a flag ‘fAllowOtherInputs’ which is already saying:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: If false, no selection process -> append all the `CoinControl` selected outpoints to the selection result (while they passed all the `AvailableCoins` checks and are available in the 'vCoins' vector).
### Changes
As can notice, the first point in the coin filtering process is duplicated in the two option flags. And the second one, is slightly different merely because it takes into account whether the coin is on the `AvailableCoins` vector or not.
So it makes sense to merge ‘m_add_inputs’ and ‘fAllowOtherInputs’ into a single field for the coin filtering process while introduce other changes to add the missing/skipped internal and external coins into 'vCoins' vector if they were manually selected by the user.
——————————————————————————————————
Just as an extra note:
On top of this, I’m working on unifying/untangling further the coin filtering and selection processes so we have less duplicate functionality in both processes.
ACKs for top commit:
laanwj:
Code review ACK d338712886
Tree-SHA512: 98920b80dd787cfe737dacd4c59575dfa8393c799b55f2aaef9aed2b15c61470715a88663557b49c7400938220f99af7690be01980a8684f4f71947407f21750
0d8e68d705 refactor: move DEFAULT_*_LIMIT assertions from validation to policy (fanquake)
9c94f3b3a7 refactor: move EXTRA_DESCENDANT_TX_SIZE_LIMIT to policy/policy.h (fanquake)
39c6036253 refactor: use braced initialization in policy/policy.h (fanquake)
01ccfbe3aa scripted-diff: use static constexpr in policy/policy.h (fanquake)
62d56bb714 refactor: Move DEFAULT_DESCENDANT_SIZE_LIMIT to policy/policy.h (fanquake)
a34aa4c187 refactor: Move DEFAULT_DESCENDANT_LIMIT to policy/policy.h (fanquake)
05fc5fdc13 refactor: Move DEFAULT_ANCESTOR_SIZE_LIMIT to policy/policy.h (fanquake)
da8d304960 refactor: Move DEFAULT_ANCESTOR_LIMIT to policy/policy.h (CAnon)
Pull request description:
Picks up #25295. Which was a follow up to [a comment in #25254](https://github.com/bitcoin/bitcoin/pull/25254#discussion_r890595318).
Moves policy constants from validation.h to policy.h.
ACKs for top commit:
laanwj:
Code review ACK 0d8e68d705
w0xlt:
reACK 0d8e68d705
darosior:
ACK 0d8e68d705
Tree-SHA512: 79900b09dc3a8020b5053ec734f462cb6e8184ed2b76e9d8afae7fe5331bbc906daaa42c0f622782797d971aaf5698aa0155511ec1d15582cc7675c271664a8d
Do the closing in `Sock::Reset()` and remove the standalone
`CloseSocket()`.
This reduces the exposure of low-level sockets (i.e. integer file
descriptors) outside of the `Sock` class.
Define (and document) `NOMINMAX` once, rather than across multiple
source files.
Defining this prevents the definition of min/max macros when using
mingw-w64, which may conflict with unprefixed std::min/max usage. While
that might not be the case for us, we'd always prefer to use the standard
library in any case.
For example:
73cadc06c6/mingw-w64-headers/include/ntdef.h (L289-L300)
Otherwise, RPC commands such as `walletcreatefundedpsbt` will not support the manual selection of locked, spent and externally added coins.
Full explanation is inside #25118 comments but brief summary is:
`vCoins` at `SelectCoins` time could not be containing the manually selected input because, even when they were selected by the user, the current `AvailableCoins` flow skips locked and spent coins.
Extra note: this is an intermediate step to unify the `fAllowOtherInputs`/`m_add_inputs` concepts. It will not be a problem anymore in the future when we finally decouple the wtx-outputs lookup process from `SelectCoins` and don't skip the user's manually selected coins in `AvailableCoins`.
Seeking to make the `CoinControl` option less confusing/redundant.
In #16377 the `CoinControl` flag ‘m_add_inputs’ was added to tell the coin filtering and selection process two things:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: Search the wtxs-outputs and append all the `CoinControl` selected outpoints to the selection result (skipping all the available output checks). Nothing else.
Meanwhile, in `CoinControl` we already have a flag ‘fAllowOtherInputs’ which is already saying:
- Coin Filtering: Only use the provided inputs. Skip the Rest.
- Coin Selection: If false, no selection process -> append all the `CoinControl` selected outpoints to the selection result (while they passed all the `AvailableCoins` checks and are available in the 'vCoins' vector).
As can notice, the first point in the coin filtering process is duplicated in the two option flags. And the second one, is slightly different merely because it takes into account whether the coin is on the `AvailableCoins` vector or not.
So it makes sense to merge ‘m_add_inputs’ and ‘fAllowOtherInputs’ into a single field for the coin filtering process while introduce other changes to add the missing/skipped coins into 'vCoins' vector if they were manually selected by the user (follow-up commits).
7ca8726f63 wallet: fix warning: "argument name 'feerate' in comment does not match parameter name" (furszy)
Pull request description:
Should solve the tiny https://github.com/bitcoin/bitcoin/pull/25005#issuecomment-1159403854.
Which merely happens for the extra "=" character after the comma.
ACKs for top commit:
Empact:
Code Review ACK 7ca8726f63
Tree-SHA512: e5368c1114f715bd93cb653c607fd0942ab0b79f709ed7aa627b3fc7e7efd096c92c5c86908c7f26c363b21e391a8faa812727eb32c285e54da3ce0429290361
fd5c996d16 wallet: GetAvailableBalance, remove double walk-through every available coin (furszy)
162d4ad10f wallet: add 'only_spendable' filter to AvailableCoins (furszy)
cdf185ccfb wallet: remove unused IsSpentKey(hash, index) method (furszy)
4b83bf8dbc wallet: avoid extra IsSpentKey -> GetWalletTx lookups (furszy)
3d8a282257 wallet: decouple IsSpentKey(scriptPubKey) from IsSpentKey(hash, n) (furszy)
a06fa94ff8 wallet: IsSpent, 'COutPoint' arg instead of (hash, index) (furszy)
91902b7720 wallet: IsLockedCoin, 'COutPoint' arg instead of (hash, index) (furszy)
9472ca0a65 wallet: AvailableCoins, don't call 'wtx.tx->vout[i]' multiple times (furszy)
4ce235ef8f wallet: return 'CoinsResult' struct in `AvailableCoins` (furszy)
Pull request description:
This started in #24845 but grew out of scope of it.
So, points tackled:
1) Avoid extra `GetWalletTx` lookups inside `AvailableCoins -> IsSpentKey`.
`IsSpentKey` was receiving the tx hash and index to internally lookup the tx inside the wallet's map. As all the `IsSpentKey` function callers already have the wtx available, them can provide the `scriptPubKey` directly.
2) Most of the time, we call `Wallet::AvailableCoins`, and later on the process, skip the non-spendable coins from the result in subsequent for-loops. So to speedup the process: introduced the ability to filter by "only_spendable" coins inside `Wallet::AvailableCoins` directly.
(the non-spendable coins skip examples are inside `AttemptSelection->GroupOutputs` and `GetAvailableBalance`).
4) Refactored `AvailableCoins` in several ways:
a) Now it will return a new struct `CoinsResult` instead of receiving the vCoins vector reference (which was being cleared at the beginning of the method anyway). --> this is coming from #24845 but cherry-picked it here too to make the following commits look nicer.
b) Unified all the 'wtx.tx->vout[I]' calls into a single call (coming from this comment https://github.com/bitcoin/bitcoin/pull/24699#discussion_r854163032).
5) The wallet `IsLockedCoin` and `IsSpent` methods now accept an `OutPoint` instead of a hash:index. Which let me cleanup a bunch of extra code.
6) Speeded up the wallet 'GetAvailableBalance': filtering `AvailableCoins` by spendable outputs only and using the 'AvailableCoins' retrieved `total_amount` instead of looping over all the retrieved coins once more.
-------------------------------------------------------
Side topic, all this process will look even nicer with #25218
ACKs for top commit:
achow101:
ACK fd5c996d16
brunoerg:
crACK fd5c996d16
w0xlt:
Code Review ACK fd5c996d16
Tree-SHA512: 376a85476f907f4f7d1fc3de74b3dbe159b8cc24687374d8739711ad202ea07a33e86f4e66dece836da3ae6985147119fe584f6e672f11d0450ba6bd165b3220
dcf36fe8e3 test: implement 'bech32m' mode for `getnewdestination()` helper (Sebastian Falbesoner)
1999dcfa40 test: add helpers for creating P2TR scripts/addresses from output key (Sebastian Falbesoner)
Pull request description:
This PR adds the missing 'bech32m' mode for the `getnewdestination()` helper and sets it as default, i.e. the function returns a tuple (output x-only-pubkey, scriptPubKey, taproot address) now if not specified otherwise. In a preparation commit, the helpers `output_key_to_p2tr{_script}` are introduced. Note that in contrast to all other common script output types, there are usually _two_ keys involved in creating a taproot output (internal key and output key), hence the prefix `output_` is used to clarify that the output key is expected and the helpers don't do any key tweaking.
Thanks to michaelfolkson (for pointing out this TODO that I forgot about) and sipa (for patiently explaining basic things about BIP341).
ACKs for top commit:
michaelfolkson:
ACK dcf36fe8e3
w0xlt:
reACK dcf36fe8e3
Tree-SHA512: 5bb8d5fd96c63092ede10c3f022ffb2e13c14e333c4aa73348d95deb70cbf0a74745218dc4a7c419eb846793dd69e8217a7b4332a13ae2b2758e100b51fb1a9f
ce1c8104aa build: Remove unused `LIBBITCOIN_KERNEL` variable (Hennadii Stepanov)
Pull request description:
Noticed that while working on moving the build system to CMake. But I [am not the first](https://github.com/bitcoin/bitcoin/pull/24322/files#r860472867) one :)
ACKs for top commit:
laanwj:
ACK ce1c8104aa
Tree-SHA512: 877b9f0d64c4c72f403335d7a8462e551f6f8cd5648a211f980d6da5ed7683521d6549f6acf15ac8e55f67915c556201a1980228c975a22135507746e2f392ce
241c4d047e doc: Correct comment describing value of MAX_FILE_SIZE_PSBT as in MiB (Ben Woosley)
64f81a38b9 doc: Correct nPruneTarget misidentifying units of variable (darosior)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/15848, darosior fixed up a comment which mis-identified the units of a constant.
Another comment misidentified a value as in MiB rather than MB.
ACKs for top commit:
laanwj:
Code review ACK 241c4d047e
darosior:
ACK 241c4d047e, with or without https://github.com/bitcoin/bitcoin/pull/25299#discussion_r892705277
Tree-SHA512: 96c03a35140e5c53759f387bd292a8f8f621ba74c3cf6621939fad40f48892d23141c747ad3ab4fd71108e3b737670175abc2eb3990a1bd1660366c55d61ddf8
f7595f1354 build: add and use CXX_STANDARD in depends (fanquake)
7e7b3e42fa build: add and use C_STANDARD in depends (fanquake)
Pull request description:
By explicitly setting a C standard version we avoid any potential for issues/differences in libraries that may come about due to C STD version, as well as avoid potentially being opted into newer code / features in libraries when compiler defaults change (i.e as of 11.0.0, Clang now defaults to gnu17 over gnu11).
This should be a no-op for our release builds, because it's just explicitly setting the default that is [already being used](https://github.com/fanquake/core-review/blob/master/compiler-defaults.md). However this is relevant for anyone building depends with a newer compiler.
I found [one broken `__STDC_VERSION__` check in the](https://github.com/miniupnp/miniupnp/pull/552) miniupnpc header.
At the same time, add `CXX_STANDARD` for setting our C++ standard, and use that over setting `-std=c++17` for cxx packages.
Guix builds:
```bash
```
ACKs for top commit:
dongcarl:
Code Review ACK f7595f1354
laanwj:
Code review ACK f7595f1354
Tree-SHA512: 9255190d91ba3de20762b1d6af35c59d64f3d77a52bbe9a3f1dfb6bcf16daef66054ebef96b58e7285cd01bf613e69a78bd5e8681c21293e254f23d1fa7b0f71
667e316bcb contrib: Update makeseeds to asmap-nextgen (laanwj)
ae00b9e02c contrib: add seeds progress indicator and remove asmap one in makeseeds script (Jon Atack)
b54180303d contrib: Use asmap for ASN lookup in makeseeds (laanwj)
Pull request description:
Add an argument `-a` to provide a asmap file to do the IP to ASN lookups.
This speeds up the script greatly, and makes the output deterministic. Also removes the dependency on `dns.lookup`.
I've annotated the output with ASxxxx comments to provide a way to verify the functionality.
For now I've added instructions in README.md to download and use the `demo.map` from the asmap repository. When we have some other mechanism for distributing asmap files we could switch to that.
This continues #24824. I've removed the fallbacks and extra complexity, as everyone will be using the same instructions anyway.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: russeree <reese.russell@ymail.com>
ACKs for top commit:
sipa:
ACK 667e316bcb
dunxen:
re-ACK 667e316
Tree-SHA512: c4cedfbd1dee6be7547aa92dd9e262c46f0ff8099e647559b2a40eab0cc9874e9a813706630dd5c880390d23f432e789fb3e7e8a09f376f567071e68f5904c65
The value was only being updated launching releases with higher version numbers
and not if the user launched a previous release.
Co-authored-by: MacroFake <falke.marco@gmail.com>
7832e9438f test: fundrawtransaction preset input weight calculation (S3RK)
c3981e379f wallet: do not count wallet utxos as external (S3RK)
Pull request description:
Correctly differentiating between external vs non-external utxos in coin control produces more accurate weight and fee estimations.
Weight for external utxos is estimated based on the maximum signature size, while for the wallet utxos we expect minimal signature due to signature grinding.
ACKs for top commit:
achow101:
re-ACK 7832e9438f
Xekyo:
re-ACK 7832e9438f
furszy:
ACK 7832e943
Tree-SHA512: bb5635b0bd85fa9a76922a53ad3fa062286424c06a695a0e87407c665713e80a33555b644fbb13bcc1ab503dcd7f53aacbdc368d69ac0ecff8005603623ac94f
6e68ccbefe net: use Sock::WaitMany() instead of CConnman::SocketEvents() (Vasil Dimov)
ae263460ba net: introduce Sock::WaitMany() (Vasil Dimov)
cc74459768 net: also wait for exceptional events in Sock::Wait() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
`Sock::Wait()` waits for IO events on one socket. Introduce a similar `virtual` method `WaitMany()` that waits simultaneously for IO events on more than one socket.
Use `WaitMany()` instead of `CConnman::SocketEvents()` (and ditch the latter). Given that the former is a `virtual` method, it can be mocked by unit and fuzz tests. This will help to make bigger parts of `CConnman` testable (unit and fuzz).
ACKs for top commit:
laanwj:
Code review ACK 6e68ccbefe
jonatack:
re-ACK 6e68ccbefe per `git range-diff e18fd47 6747729 6e68ccb`, and verified rebase to master and debug build
Tree-SHA512: 917fb6ad880d64d3af1ebb301c06fbd01afd8ff043f49e4055a088ebed6affb7ffe1dcf59292d822f10de5f323b6d52d557cb081dd7434634995f9148efcf08f
d273e53b6e bench/rpc_mempool: Create ChainTestingSetup, use its CTxMemPool (Carl Dong)
020caba3df bench: Use existing CTxMemPool in TestingSetup (Carl Dong)
86e732def3 scripted-diff: test: Use CTxMemPool in TestingSetup (Carl Dong)
213457e170 test/policyestimator: Use ChainTestingSetup's CTxMemPool (Carl Dong)
319f0ceeeb rest/getutxos: Don't construct empty mempool (Carl Dong)
03574b956a tree-wide: clang-format CTxMemPool references (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
This PR reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from `ArgsManager`, eventually all of libbitcoinkernel will be decoupled from `ArgsManager`.
The changes in this PR:
- Allows us to have less code churn as we modify `CTxMemPool`'s constructor in later PRs
- In many cases, we can make use of existing `CTxMemPool` instances, getting rid of extraneous constructions
- In other cases, we construct a `ChainTestingSetup` and use the `CTxMemPool` there, so that we can rely on the logic in `setup_common` to set things up correctly
## Notes for Reviewers
### A note on using existing mempools
When evaluating whether or not it's appropriate to use an existing mempool in a `*TestingSetup` struct, the key is to make sure that the mempool has the same lifetime as the `*TestingSetup` struct.
Example 1: In [`src/fuzz/tx_pool.cpp`](b4f686952a/src/test/fuzz/tx_pool.cpp), the `TestingSetup` is initialized in `initialize_tx_pool` and lives as a static global, while the `CTxMemPool` is in the `tx_pool_standard` fuzz target, meaning that each time the `tx_pool_standard` fuzz target gets run, a new `CTxMemPool` is created. If we were to use the static global `TestingSetup`'s CTxMemPool we might run into problems since its `CTxMemPool` will carry state between subsequent runs. This is why we don't modify `src/fuzz/tx_pool.cpp` in this PR.
Example 2: In [`src/bench/mempool_eviction.cpp`](b4f686952a/src/bench/mempool_eviction.cpp), we see that the `TestingSetup` is in the same scope as the constructed `CTxMemPool`, so it is safe to use its `CTxMemPool`.
### A note on checking `CTxMemPool` ctor call sites
After the "tree-wide: clang-format CTxMemPool references" commit, you can find all `CTxMemPool` ctor call sites with the following command:
```sh
git grep -E -e 'make_unique<CTxMemPool>' \
-e '\bCTxMemPool\s+[^({;]+[({]' \
-e '\bCTxMemPool\s+[^;]+;' \
-e '\bnew\s+CTxMemPool\b'
```
At the end of the PR, you will find that there are still quite a few call sites that we can seemingly get rid of:
```sh
$ git grep -E -e 'make_unique<CTxMemPool>' -e '\bCTxMemPool\s+[^({;]+[({]' -e '\bCTxMemPool\s+[^;]+;' -e '\bnew\s+CTxMemPool\b'
# rearranged for easier explication
src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
src/rpc/mining.cpp: CTxMemPool empty_mempool;
src/test/util/setup_common.cpp: CTxMemPool empty_pool;
src/bench/mempool_stress.cpp: CTxMemPool pool;
src/bench/mempool_stress.cpp: CTxMemPool pool;
src/test/fuzz/rbf.cpp: CTxMemPool pool;
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{};
src/txmempool.h: /** Create a new CTxMemPool.
```
Let's break them down one by one:
```
src/init.cpp: node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
src/test/util/setup_common.cpp: m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
```
Necessary
-----
```
src/rpc/mining.cpp: CTxMemPool empty_mempool;
src/test/util/setup_common.cpp: CTxMemPool empty_pool;
```
These are fixed in #25223 where we stop requiring the `BlockAssembler` to have a `CTxMemPool` if it's not going to consult it anyway (as is the case in these two call sites)
-----
```
src/bench/mempool_stress.cpp: CTxMemPool pool;
src/bench/mempool_stress.cpp: CTxMemPool pool;
```
Fixed in #24927.
-----
```
src/test/fuzz/rbf.cpp: CTxMemPool pool;
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/tx_pool.cpp: CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
src/test/fuzz/validation_load_mempool.cpp: CTxMemPool pool{};
```
These are all cases where we don't want the `CTxMemPool` state to persist between runs, see the previous section "A note on using existing mempools"
-----
```
src/txmempool.h: /** Create a new CTxMemPool.
```
It's a comment (someone link me to a grep that understands syntax plz thx)
ACKs for top commit:
laanwj:
Code review ACK d273e53b6e
Tree-SHA512: c4ff3d23217a7cc4a7145defc7b901725073ef73bcac3a252ed75f672c87e98ca0368d1d8c3f606b5b49f641e7d8387d26ef802141b650b215876f191fb6d5f9
f0f5cd79b5 Bugfix: configure: Define default for use_libevent (Luke Dashjr)
Pull request description:
Another trivial fix like #25051 - I think this is the only other one missing.
ACKs for top commit:
laanwj:
Code review ACK f0f5cd79b5
Tree-SHA512: 888c2e6d032ef1de5af635e2a9b2b8ab560c86bd10a6cee54aa9aa62ae43f03c19889bb6a2b64cf8982d4cd514f97ca3ed743c71ed0651e9295a4b1726955b9b
