32748da0f4 whitespace fixups after move and scripted-diff (glozow)
fa47622e8d scripted-diff: rename variables in policy/rbf (glozow)
ac761f0a23 MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf (glozow)
9c2f9f8984 MOVEONLY: check that fees > direct conflicts to policy/rbf (glozow)
3f033f01a6 MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf (glozow)
7b60c02b7d MOVEONLY: BIP125 Rule 2 to policy/rbf (glozow)
f8ad2a57c6 Make GetEntriesForConflicts return std::optional (glozow)
Pull request description:
This PR does not change behavior. It extracts the BIP125 logic into helper functions (and puts them in the policy/rbf* files). This enables three things - I think each one individually is pretty good:
- Implementation of package RBF (see #22290). I want it to be as close to BIP125 as possible so that it doesn't become a distinct fee-bumping mechanism. Doing these move-only commits first means the diff is mostly mechanical to review, and I just need to create a function that mirrors the single transaction validation.
- We will be able to isolate and test our RBF logic alone. Recently, there have been some discussions on discrepancies between our code and BIP125, as well as proposals for improving it. Generally, I think making this code more modular and de-bloating validation.cpp is probably a good idea.
- Witness Replacement (replacing same-txid-different-wtxid when the witness is significantly smaller and therefore higher feerate) in a BIP125-similar way. Hopefully it can just be implemented with calls to the rbf functions (i.e. `PaysForRBF`) and an edit to the relevant mempool entries.
ACKs for top commit:
mjdietzx:
ACK 32748da0f4
theStack:
Code-review ACK 32748da0f4📐
MarcoFalke:
review ACK 32748da0f4🦇
Tree-SHA512: d89985c8b4b42b54861018deb89468e04968c85a3fb1113bbcb2eb2609577bc4fd9bf254593b5bd0e7ab059a0fa8192d1a903b00f77e6f120c7a80488ffcbfc0
853c4edb70 [net] Remove asmap argument from CNode::CopyStats() (John Newbery)
9fd5618610 [asmap] Make DecodeAsmap() a utility function (John Newbery)
bfdf4ef334 [asmap] Remove SanityCheckASMap() from netaddress (John Newbery)
07a9eccb60 [net] Remove CConnman::Options.m_asmap (John Newbery)
Pull request description:
These small cleanups to the asmap code are the first 4 commits from #22910. They're minor improvements that are independently useful whether or not 22910 is merged.
ACKs for top commit:
naumenkogs:
ACK 853c4edb70
theStack:
Concept and code-review ACK 853c4edb70🗺️
fanquake:
ACK 853c4edb70
Tree-SHA512: 64783743182592ac165df6ff8d18870b63861e9204ed722c207fca6938687aac43232a5ac4d8228cf8b92130ab0349de1b410a2467bb5a9d60dd9a7221b3b85b
f530202353 Make unexpected time type in BCLog::LogMsg() a compile-time error (Martin Ankerl)
bddae7e7ff Add util/types.h with ALWAYS_FALSE template (MarcoFalke)
498b323425 log, timer: improve BCLog::LogMsg() (Jon Atack)
8d2f847ed9 sync: inline lock contention logging macro to fix time duration (Jon Atack)
Pull request description:
Follow-up to #22736.
The first commit addresses the issue identified and reported by Martin Ankerl in https://github.com/bitcoin/bitcoin/pull/22736#discussion_r703019629 to fix the lock contention duration reporting.
The next three commits make improvements to the timer code in `BCLog::LogMsg()` and add `util/types.h` with an `ALWAYS_FALSE` template, that springboard from https://github.com/bitcoin/bitcoin/pull/22736#discussion_r702747920 by Marco Falke.
ACKs for top commit:
martinus:
re-ACK f530202353. I ran a fully synced node for about a day. My node was mostly idle though so not much was going on. I [wrote a little script](https://github.com/martinus/bitcoin-stuff/blob/main/scripts/parse-debuglog-contention-single.rb) to parse the `debug.log` and summarize the output to see if anything interesting was going on, here is the result:
theStack:
ACK f530202353
Tree-SHA512: 37d093eac5590e1b5846ab5994d0950d71e131177d1afe4a5f7fcd614270f977e0ea117e7af788e9a74ddcccab35b42ec8fa4db3a3378940d4988df7d21cdaaa
fdd71448e7 system: skip trying to set the locale on NetBSD (fanquake)
Pull request description:
Just treat it the same as the other BSDs.
Fixes#17379.
ACKs for top commit:
laanwj:
Code review ACK fdd71448e7
practicalswift:
cr ACK fdd71448e7
Tree-SHA512: 5fe0a66f014279ad2683b548692a36af493377fb92d1f28b15dc4feef871190fe08ef40dcc4f5ba21a525fe365c42fb429fe4be0673a1e96db163af587c23204
DecopeAsmap is a pure utility function and doesn't have any
dependencies on addrman, so move it to util/asmap.
Reviewer hint: use:
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
2b071265c3 error if settings.json exists, but is unreadable (Tyler Chambers)
Pull request description:
If settings.json exists, but is unreadable, we should error instead of overwriting.
Fixes#22571
ACKs for top commit:
Zero-1729:
tACK 2b071265c3
ShaMan239:
tACK 2b071265c3
prayank23:
tACK 2b071265c3
ryanofsky:
Code review ACK 2b071265c3. Thanks for the fix! Note that PR https://github.com/bitcoin-core/gui/pull/379 will change the appearance of dialogs shown in screenshots above. So it could be interesting to test the two PRs together (but current testing seems more than sufficient)
theStack:
ACK 2b071265c3📁
Tree-SHA512: 6f7f96ce8a13213d0335198a2245d127264495c877105058d1503252435915b332a6e55068ac21088f4c0c017d564689f4956213328d5bdee81d73711efc5511
f293c68be0 MOVEONLY: getting mempool conflicts to policy/rbf (glozow)
8d71796335 [validation] quit RBF logic earlier and separate loops (glozow)
badb9b11a6 call SignalsOptInRBF instead of checking all inputs (glozow)
e0df41d7d5 [validation] default conflicting fees and size to 0 (glozow)
b001b9f6de 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 f293c68be0
theStack:
Code-review ACK f293c68be0📔
ariard:
ACK f293c68b
Tree-SHA512: a60370994569cfc91d4b2ad5e94542d4855a48927ae8b174880216074e4fa50d4523dd4ee36efdd6edf2bf7adb87a8beff9c3aaaf6dd323b286b287233e63790
4c69571e6e doc: remove outdated comment (Martin Zumsande)
16652a93ea refactor: Remove unused KeyIDHasher (Martin Zumsande)
Pull request description:
Small follow-ups to #19935:
- Removal of unused `KeyIDHasher` class ([comment in 19935](https://github.com/bitcoin/bitcoin/pull/19935#discussion_r544464524))
- Removal of an outdated comment, which referred to an old problem with the no longer supported Boost 1.46 and `boost::unordered_map`, now replaced by `std::unordered_map`. ([comment in 19935](https://github.com/bitcoin/bitcoin/pull/19935#discussion_r540911134))
ACKs for top commit:
Saviour1001:
Tested ACK <code>[4c69571](4c69571e6e)</code>
Zero-1729:
ACK 4c69571e6e
theStack:
ACK 4c69571e6e🆗
Tree-SHA512: 243fda2120bfac6c40a268ca2c0f34482ce27e71fbc50005c0d13c2ad5db9ee72a037f9937c37cc50ed0f9f6f11ee6afee4ac50e5031d6876ec942f41f38dadf
127b4608e9 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6 util: Check if specified config file cannot be opened (nthumann)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22612.
When running e.g. `./src/bitcoind -datadir=/tmp/bitcoin -regtest -conf=/tmp/bitcoin/regtest/bitcoin.conf` and the specified config cannot be opened (doesn't exist, permission denied, ...), the initialization silently uses the default config.
As voidburn already noted:
> I can't think of a situation in which a config file is specified explicitly (in the startup options, as per service unit linked above), but inaccessible, where the fail condition should be to keep booting using defaults instead.
With this patch applied, the initialization will fail immediately, if the specified config file cannot be opened. If no config file is explicitly specified, the behavior is unchanged. This not only affects `bitcoind`, but also `bitcoin-cli` and `bitcoin-qt`.
In the example below the datadir is accessible, but the config file is not due to insufficient permissions:
```
$ ./src/bitcoind -datadir=/tmp/bitcoin -regtest --debug=1 -conf=/tmp/bitcoin/regtest/bitcoin.conf
Error: Error reading configuration file: specified config file "/tmp/bitcoin/regtest/bitcoin.conf" could not be opened.
```
ACKs for top commit:
0xB10C:
ACK 127b4608e9
Zero-1729:
tACK 127b4608e9
theStack:
Tested ACK 127b4608e9
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
bb56486a17 refactor: Reuse MakeUnorderedList where possible (Hennadii Stepanov)
77a90f03ac refactor: Move MakeUnorderedList into util/string.h to make it reusable (Hennadii Stepanov)
6a5ccd65c7 scripted-diff: Rename JoinErrors in more general MakeUnorderedList (Hennadii Stepanov)
Pull request description:
A nice `JoinErrors` utility function was introduced in https://github.com/bitcoin-core/gui/pull/379 by Russell Yanofsky.
This PR renames this function and re-uses it across the code base.
ACKs for top commit:
Zero-1729:
Concept ACK bb56486a17
theStack:
Code-review ACK bb56486a17
Talkless:
utACK bb56486a17
ryanofsky:
Code review ACK bb56486a17. Nice deduping, thanks for this!
Tree-SHA512: 6bdbfa61f2ffa69e075f46b733f247c6d5b8486779a1dac064285a199a4bb8bc5ef44eaee37086305646b5c88eb6a11990883219a4a9140a5117ee21ed529bb9
92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf
meshcollider:
Code review ACK 92993aa5cf
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
This adds a new descriptor with syntax e.g. tr(KEY,{S1,{{S2,S3},S4})
where KEY is a key expression for the internal key and S_i are
script expression for the leaves. They have to be organized in
nested {A,B} groups, with exactly two elements.
tr() only exists at the top level, and inside the script expressions
only pk() scripts are allowed for now.
aca0e5dcdb Remove `GetDataDir(bool fNetSpecific = true)` function (Kiminuo)
b3e67f20a0 scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
4c3a5dcbfc scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
13bd8bb053 Make `ArgsManager.GetDataDirPath` private and drop needless suffix (Kiminuo)
4d8189f620 scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.GetDataDirBase()` in tests (Kiminuo)
0f53df47d5 Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` (Kiminuo)
716de29dd8 Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. (Kiminuo)
Pull request description:
This PR is a follow up PR to #21244. The PR attempts to move us an inch towards the [goal](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615307465) by removing `GetDataDir(net_specific)` and replacing it by `gArgs.GetDataDir(net_specific)` calls.
The approach of this PR attempts to be similar to the one chosen in "De-globalize ChainstateManager" (#20158). The goal is to pass `ArgsManager` to functions (or ideally to have `ArgsManager` as a member of a class where needed; inspiration from here: #21789) instead of having it as a global variable (i.e. `gArgs`).
**Notes:**
* First commit makes `m_cached_blocks_path` `mutable` as was suggested [here](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615274095) but not fully applied in #21244. (`m_cached_datadir_path` and `m_cached_network_datadir_path` were marked as `mutable` in #21244) This commit can be in a separate PR too.
* Other commits deal with removing of `GetDataDir(net_specific)` function.
* This was originally part of #21244 but it was [left]((https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-633779754)) for a follow up PR.
* I think that the proposed changes show nicely where there is reliance on `gArgs` which is IMO a good thing.
If you know about a better approach how to do this, please share it here.
ACKs for top commit:
hebasto:
ACK aca0e5dcdb
MarcoFalke:
re-ACK aca0e5dcdb👃
Tree-SHA512: deec4d88edb32d7f4c818c3a74ffbb64709685819b88242dcf5dbaa1fb611f3ce2b29d2576ddb9e0dc5e75288e43538968224008c0a80e7149fc81c309f7c9da
e286cd0d7b net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)
Pull request description:
Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in https://github.com/bitcoin/bitcoin/pull/21631.
ACKs for top commit:
practicalswift:
cr ACK e286cd0d7b: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
laanwj:
Code review ACK e286cd0d7b
Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
74bf850ac4 faster HexStr => 13% faster blockToJSON (Martin Ankerl)
Pull request description:
`std::string`'s push_back is rather slow because it needs to check & update the string size. For
`HexStr` the output string size is already easily know, so we can initially create the string with
the correct size and then just assign the data.
`HexStr` is heavily usd in `blockToJSON`, so this change is a noticeable benefit. Benchmark on an i7-8700 @3.2GHz:
* 71,315,461.00 ns/op master
* 62,842,490.00 ns/op this commit
So this little change makes `blockToJSON` about ~13% faster.
ACKs for top commit:
laanwj:
Code review ACK 74bf850ac4
theStack:
re-ACK 74bf850ac4
Tree-SHA512: fc99105123edc11f4e40ed77aea80cf7f32e49c53369aa364b38395dcb48575e15040b0489ed30d0fe857c032a04e225c33e9d95cdfa109a3cb5a6ec9a972415
fa340b8794 refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash (MarcoFalke)
fae33f98e6 Fix assumeutxo crash due to invalid base_blockhash (MarcoFalke)
fa5668bfb3 refactor: Use type-safe assumeutxo hash (MarcoFalke)
0000007709 refactor: Remove unused code (MarcoFalke)
faa921f787 move-only: Add util/hash_type (MarcoFalke)
Pull request description:
Starting with commit d6af06d68a, a block hash of all-zeros is invalid and will lead to a crash of the node. Can be tested by cherry-picking the test changes without the other changes.
Stack trace (copied from https://github.com/bitcoin/bitcoin/pull/21584#discussion_r612673879):
```
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff583c8b1 in __GI_abort () at abort.c:79
#2 0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89,
function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
#3 0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89,
function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
#4 0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
#5 0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
#6 0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
#7 0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
at validation.cpp:5422
#8 0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
#9 0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=...,
root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
#10 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
at test/validation_chainstatemanager_tests.cpp:262
ACKs for top commit:
laanwj:
Code review re-ACK fa340b8794
jamesob:
ACK fa340b8794 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))
Tree-SHA512: c2c4e66c1abfd400ef18a04f22fec1f302f1ff4d27a18050f492f688319deb4ccdd165ff792eee0a1f816e7b69fb64080662b79517ab669e3d26b9eb77802851
