fa8e76bb90 wallet: Add sanity checks to AntiFeeSnipe (MarcoFalke)
Pull request description:
I added those sanity checks as part of implementing BIP 326, but I think they make sense on their own. The checks require the transaction to be passed in to `DiscourageFeeSniping`. Also, replace `(int)locktime` cast with the equivalent `int(locktime)` cast.
ACKs for top commit:
chris-belcher:
ACK fa8e76bb90
S3RK:
Code review ACK fa8e76bb90
achow101:
ACK fa8e76bb90
w0xlt:
Code Review ACK fa8e76bb90
Tree-SHA512: 6fe37c85f074851ef09cae8addcb836257089290fecec515c129c3180e9ccb64740aaac76043af10ce7c213e5001568ca6b4b62ae9631f0994ed676b07118074
61152183ab wallet: Add a deprecation warning for newly created legacy wallets (Andrew Chow)
Pull request description:
As we slowly deprecate legacy wallets, we need to warn users that are making new legacy wallets that their wallet type is going to be unsupported in the future.
ACKs for top commit:
jonatack:
ACK 61152183ab
S3RK:
reACK 61152183ab
theStack:
ACK 61152183ab
Tree-SHA512: e89bfb8168869542498958f0c9a2ab302dfd43287f8a49e7d9e09f60438a567bb8b7219a4e569797ee819b30b624f532fcc0b70c6aa0edcb392a301b8ce8b541
5d7c69b887 rpc: rename getdeploymentinfo status-next to status_next (Jon Atack)
Pull request description:
Rename the `status-next` field to `status_next` in getdeploymentinfo before the RPC is released in v23.
Before
```
Result:
{ (json object)
"hash" : "str", (string) requested block hash (or tip)
"height" : n, (numeric) requested block height (or tip)
"deployments" : { (json object)
"xxxx" : { (json object) name of the deployment
"type" : "str", (string) one of "buried", "bip9"
"height" : n, (numeric, optional) height of the first block which the rules are or will be enforced (only for "buried" type, or "bip9" type with "active" status)
"active" : true|false, (boolean) true if the rules are enforced for the mempool and the next block
"bip9" : { (json object, optional) status of bip9 softforks (only for "bip9" type)
"bit" : n, (numeric, optional) the bit (0-28) in the block version field used to signal this softfork (only for "started" and "locked_in" status)
"start_time" : xxx, (numeric) the minimum median time past of a block at which the bit gains its meaning
"timeout" : xxx, (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in
"min_activation_height" : n, (numeric) minimum height of blocks for which the rules may be enforced
"status" : "str", (string) status of deployment at specified block (one of "defined", "started", "locked_in", "active", "failed")
"since" : n, (numeric) height of the first block to which the status applies
"status-next" : "str", (string) status of deployment at the next block
"statistics" : { (json object, optional) numeric statistics about signalling for a softfork (only for "started" and "locked_in" status)
"period" : n, (numeric) the length in blocks of the signalling period
"threshold" : n, (numeric, optional) the number of blocks with the version bit set required to activate the feature (only for "started" status)
"elapsed" : n, (numeric) the number of blocks elapsed since the beginning of the current period
"count" : n, (numeric) the number of blocks with the version bit set in the current period
"possible" : true|false (boolean, optional) returns false if there are not enough blocks left in this period to pass activation threshold (only for "started" status)
},
"signalling" : "str" (string) indicates blocks that signalled with a # and blocks that did not with a -
}
}
}
}
```
After
```
Result:
{ (json object)
"hash" : "str", (string) requested block hash (or tip)
"height" : n, (numeric) requested block height (or tip)
"deployments" : { (json object)
"xxxx" : { (json object) name of the deployment
"type" : "str", (string) one of "buried", "bip9"
"height" : n, (numeric, optional) height of the first block which the rules are or will be enforced (only for "buried" type, or "bip9" type with "active" status)
"active" : true|false, (boolean) true if the rules are enforced for the mempool and the next block
"bip9" : { (json object, optional) status of bip9 softforks (only for "bip9" type)
"bit" : n, (numeric, optional) the bit (0-28) in the block version field used to signal this softfork (only for "started" and "locked_in" status)
"start_time" : xxx, (numeric) the minimum median time past of a block at which the bit gains its meaning
"timeout" : xxx, (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in
"min_activation_height" : n, (numeric) minimum height of blocks for which the rules may be enforced
"status" : "str", (string) status of deployment at specified block (one of "defined", "started", "locked_in", "active", "failed")
"since" : n, (numeric) height of the first block to which the status applies
"status_next" : "str", (string) status of deployment at the next block
"statistics" : { (json object, optional) numeric statistics about signalling for a softfork (only for "started" and "locked_in" status)
"period" : n, (numeric) the length in blocks of the signalling period
"threshold" : n, (numeric, optional) the number of blocks with the version bit set required to activate the feature (only for "started" status)
"elapsed" : n, (numeric) the number of blocks elapsed since the beginning of the current period
"count" : n, (numeric) the number of blocks with the version bit set in the current period
"possible" : true|false (boolean, optional) returns false if there are not enough blocks left in this period to pass activation threshold (only for "started" status)
},
"signalling" : "str" (string) indicates blocks that signalled with a # and blocks that did not with a -
}
}
}
}
```
Top commit has no ACKs.
Tree-SHA512: 4facfd7af3cfb7b6f5495758c4387602802f5e39d9270b162d17350a7f954eab0b74d895f17f0d8dfbc7814d36db7cff56d08c42728432885ea6f4e37aea4aa8
5ce3057c8e test: set segwit height back to 0 on regtest (Martin Zumsande)
Pull request description:
The change of `consensus.SegwitHeight` from 0 to 1 for regtest in #22818 had the effect that if I create a regtest enviroment with current master (or 23.x), and then try to load this chain with an older version (22.x), I get an InitError
`Witness data for blocks after height 0 requires validation. Please restart with -reindex`
and have to reindex because `BLOCK_OPT_WITNESS` is no longer set for the Genesis block and `NeedsRedownload()` in validation returns `true` with an older version.
That might be a bit annoying for tests that use a shared regtest dir with different versions.
If people think this is enough of an issue to be worth fixing, I think it should also make it into 23.x
ACKs for top commit:
theStack:
Concept and code-review ACK 5ce3057c8e
Tree-SHA512: b0e89ff7fc953bc0ae929d2da44cde7149321d987fb4763934f6c9635d00d807129a50b459cc5e69e86bb1819e4b063b969486e8016a1cb8db8f905fa315653d
fae20e6b50 Revert "Avoid the use of P0083R3 std::set::merge" (MarcoFalke)
fab53b5fd4 ci/doc: Set minimum required clang/libc++ version to 8.0 (MarcoFalke)
Pull request description:
This is not for 23.0, but for 24.0. It comes with the following benefits:
* Can use C++17 P0083R3 std::set::merge from libc++ 8.0
* No longer need to provide support for clang-7, which already fails to compile on some architectures (https://github.com/bitcoin/bitcoin/issues/21294#issuecomment-998098483)
This should be fine, given that all supported operating systems ship with at least clang-10:
* CentOS 8: clang-12
* Stretch: https://packages.debian.org/stretch/clang-11
* Buster: https://packages.debian.org/buster-backports/clang-11
* Bionic: https://packages.ubuntu.com/bionic-updates/clang-10
* Focal: https://packages.ubuntu.com/focal/clang-10
ACKs for top commit:
fanquake:
ACK fae20e6b50 - I think this is fine to do. I would be surprised if in another 6 months time someone was stuck on a system we supported, needing to compile Core, and only had access to Clang 7 or older. As mentioned in the PR description, all systems we currently support, already support multiple newer versions of Clang.
hebasto:
ACK fae20e6b50, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 3b4c6c130ff40dd7e84934af076863415e5dd661d823c72e3e3832566c65be6e877a7ef9164bbcf394bcea4b897fc29a48db0f231c22ace0e2c9b5638659a628
ec7d73628a [wallet] assert BnB internally calculated waste is the same as GetSelectionWaste() (glozow)
Pull request description:
#22009 introduced a `GetSelectionWaste()` function to determine how much "waste" a coin selection solution has, and is a mirror of the waste calculated inside of `SelectCoinsBnB()`. It would be bad for these two waste metrics to deviate, since it could negatively affect how often we select the BnB solution. Add an assertion to help tests catch a potential accidental change.
ACKs for top commit:
achow101:
ACK ec7d73628a
Xekyo:
ACK ec7d73628a
Tree-SHA512: 3ab7ad45ae92e7ce3f21824fb975105b6be8382edf47c252df5d13d973a3abdcb675132d223b42fcbb669cca879672c904b8a58d0676e12bf381a9219f4db37c
e8272024ab doc: Use human-friendly DefaultHint for change_address/changeAddress in wallet RPC help (Luke Dashjr)
9d5e693c9d Bugfix: doc: Correct type of change_address/changeAddress in wallet RPC help (STR, not STR_HEX) (Luke Dashjr)
Pull request description:
ACKs for top commit:
achow101:
ACK e8272024ab
Tree-SHA512: da4db2b241160c93ea66f8c572c69d4688f52a5fd8c32b66b1192925fcb360baf91be9771eaca178f5b08e1920559174260ed57caddcffade48156ec0c83c0bc
40e871d9b4 [miner] always assume we can create witness blocks (glozow)
Pull request description:
Given the low possibility of a reorg reverting the segwit soft fork, there is no longer a need to check whether segwit is active to see if it's okay to add to the block template (see also #23512, #21009, etc). `TestBlockValidity()` is also run on the block template at the end of `CreateNewBlock()`, so any invalid block would be caught there.
ACKs for top commit:
gruve-p:
ACK 40e871d9b4
jnewbery:
utACK 40e871d9b4, although I disagree about changing the test for segwit transaction in mempool before activagtion, instead of just removing it: https://github.com/bitcoin/bitcoin/pull/24421#discussion_r822933721.
achow101:
ACK 40e871d9b4
theStack:
Code-review ACK 40e871d9b4
Tree-SHA512: bf4860bf2bed8339622d05228d11d60286edb0c32a9a3c434b8d154913c07ea56e50649f4af7009c2a1c6a58a81d2299ab43b41a6f16dee7d08cc89cc1603019
These two implementations of waste calculation should never deviate.
Still keep the SelectCoinsBnB internal calculation because incremental
calculate-as-you-go is much more performant than calling
GetSelectionWaste() over and over again.
This was changed in #22818 from 0 to 1. Since it changes
BLOCK_OPT_WIT of the genesis block, older versions of bitcoin
core would not read regtest directories created with newer versions
without a reindex.
b7dbc83f23 qt: Remove network detection based on address in BIP21 (laanwj)
Pull request description:
This is removes some ugly and brittle code that switches the global network to testnet based on a provided address. I think in practice it's very unlikely for testnet BIP21 payment URIs to be used, and if so it's for testing so it's easy enough to manually copy it. Or to specify `-testnet` explicitly.
There is already no such case for `-regtest` or `-signet`.
After this change it will only accept addresses for the explicitly selected network. Others will result in a "wrong network" popup.
There is also a possibility for refactor after this as the initialization order of `PaymentServer::ipcParseCommandLine` isn't important anymore (well, it still has to be before `PaymentServer::ipcSendCommandLine`, maybe even merged with it), but I have not done so here.
ACKs for top commit:
jonatack:
ACK b7dbc83f23
achow101:
ACK b7dbc83f23
Tree-SHA512: ebc77c0e5c98f53fe254bcd0f94c9d1c06937b794346e95b14158860d5c607515e71d73b782d2726674dce86d6d4001085d473c6d1bc11c5e6c25ff3fb3cfa22
faa1aec26b Remove confusing P1008R1 violation in ATMPArgs (MarcoFalke)
Pull request description:
The `= delete` doesn't achieve the stated goal and it is also redundant, since it is not possible to default construct the `ATMPArgs` type.
This can be tested with:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 2813b62462..1c939c0b8a 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -519,6 +519,7 @@ public:
/** Parameters for child-with-unconfirmed-parents package validation. */
static ATMPArgs PackageChildWithParents(const CChainParams& chainparams, int64_t accept_time,
std::vector<COutPoint>& coins_to_uncache) {
+ ATMPArgs{};
return ATMPArgs{/* m_chainparams */ chainparams,
/* m_accept_time */ accept_time,
/* m_bypass_limits */ false,
```
Which fails on current master *and* this pull with the following error:
```
validation.cpp:525:22: error: reference member of type 'const CChainParams &' uninitialized
ATMPArgs{};
~^
validation.cpp:470:29: note: uninitialized reference member is here
const CChainParams& m_chainparams;
^
1 error generated.
```
Further reading (optional):
* http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf
ACKs for top commit:
achow101:
ACK faa1aec26b
glozow:
code review ACK faa1aec26b
Tree-SHA512: 16db2c9959a1996eafbfa533dc4d1483761b9d28295aed5a82b86abd7268da37c51c59ddc67c205165ecb415dbe637b12a0e1b3234d50ab0b3b79de66d7bd73e
8ea6167099 wallet: refactor: dedup sqlite blob binding (Sebastian Falbesoner)
Pull request description:
This refactoring PR deduplicates repeated SQLite blob binding to a statement with a newly introduced helper function `BindBlobToStatement`, abstracting away the calls to `sqlite3_bind_blob(...)`.
This should be more readable and less error-prone, e.g. in case that the error handling has to be adapted. As a slight drawback, the function where the binding happens is not printed anymore (`__func__`), i.e. one could argue this is not strictly a refactoring, but IMHO the advantages of deduplication outweigh this; binding errors are purely internal logic errors (wrong use of the sqlite API) rather than something that is dependend on external data like DB content.
ACKs for top commit:
laanwj:
Code review ACK 8ea6167099
achow101:
ACK 8ea6167099
klementtan:
ACK 8ea6167099
Tree-SHA512: 1de0d214f836bc405a01e98a3a2d71f2deaddc7d23c31cad80219d1614bec92619c06d9a4a091dd563d3e95ffb879649c29745d8f89669b2a5330552c212af3f
This is some very ugly and brittle code that switches the global network
based on a provided address, remove it. I think in practice it's very
unlikely for testnet BIP21 payment URIs to be used, and if so it's for
testing so it's easy enough to manually copy it. Or to specify
`-testnet` explicitly.
There is already no case for `-regtest` or `-signet`.
2f5fd3cf92 test: Correctly decode UTF-8 literal string paths (Ryan Ofsky)
Pull request description:
Call `fs::u8path()` to convert some UTF-8 string literals to paths, instead of relying on the implicit conversion. Fake Macro pointed out in https://github.com/bitcoin/bitcoin/pull/24306#discussion_r818566106 that `fs_tests` are incorrectly decoding some literal UTF-8 paths using the current windows codepage, instead of treating them as UTF-8. This could cause test failures depending what environment windows tests are run under.
The `fs::path` class exists to avoid problems like this, but because it is lenient with `const char*` conversions, under assumption that they are ["safe as long as the literals are ASCII"](727b0cb592/src/fs.h (L39)), bugs like this are still possible.
If we think this is a concern, followup options to try to prevent this bug in the future are:
0. Do nothing
1. Improve the "safe as long as the literals are ASCII" comment. Make it clear that non-ASCII strings are invalid.
2. Drop the implicit `const char*` conversion functions. This would be nice because it would simplifify the `fs::path` class a little, while making it safer. Drawback is that it would require some more verbosity from callers. For example, instead of `GetDataDirNet() / "mempool.dat"` they would have to write `GetDataDirNet() / fs::u8path("mempool.dat")`
3. Keep the implicit `const char*` conversion functions, but make them call `fs::u8path()` internally. Change the "safe as long as the literals are *ASCII*" comment to "safe as long as the literals are *UTF-8*".
I'd be happy with 0, 1, or 2. I'd be a little resistant to 3 even though it was would add more safety, because it would slightly increase complexity, and because I think it would encourage representing paths as strings, when I think there are so many footguns associated with paths as strings, that it's best to convert strings to paths at the earliest point possible, and convert paths to strings at the latest point possible.
ACKs for top commit:
laanwj:
Code review ACK 2f5fd3cf92
w0xlt:
crACK 2f5fd3c
Tree-SHA512: 9c56714744592094d873b79843b526d20f31ed05eff957d698368d66025764eae8bfd5305d5f7b6cc38803f0d85fa5552003e5c6cacf1e076ea6d313bcbc960c
URLs may contain a query string (prefixed with '?') and this should be ignored when parsing
the data format.
To facilitate testing this functionality, ParseDataFormat has been made non-static.
As RetFormat is now exposed in a header, it is renamed to the more
understandable RESTResponseFormat
-BEGIN VERIFY SCRIPT-
s() { sed -i 's/RetFormat/RESTResponseFormat/g' $1; }
s src/rest.cpp
s src/rest.h
-END VERIFY SCRIPT-
a84650ebd5 util: Fix ReadBinaryFile reading beyond maxsize (klementtan)
Pull request description:
Currently `ReadBinaryFile` will read beyond `maxsize` if `maxsize` is not a multiple of `128` (size of buffer)
This is due to `fread` being called with `count = 128` instead of `count = min(128, maxsize - retval.size()` at every iteration
The following unit test will fail:
```cpp
BOOST_AUTO_TEST_CASE(util_ReadWriteFile)
{
fs::path tmpfolder = m_args.GetDataDirBase();
fs::path tmpfile = tmpfolder / "read_binary.dat";
std::string expected_text(300,'c');
{
std::ofstream file{tmpfile};
file << expected_text;
}
{
// read half the contents in file
auto [valid, text] = ReadBinaryFile(tmpfile, expected_text.size() / 2);
BOOST_CHECK_EQUAL(text.size(), 150);
}
}
```
Error:
```
test/util_tests.cpp:2593: error: in "util_tests/util_ReadWriteFile": check text.size() == 150 has failed [256 != 150]
```
ACKs for top commit:
laanwj:
Code review ACK a84650ebd5
theStack:
Code-review ACK a84650ebd5
Tree-SHA512: 752eebe58bc2102dec199b6775f8c3304d899f0ce36d6a022a58e27b076ba945ccd572858b19137b769effd8c6de73a9277f641be24dfb17657fb7173ea0eda0
5b1aae12ca qt: Avoid crash on startup if int specified in settings.json (Ryan Ofsky)
84b0973e35 test: Add tests for GetArg methods / settings.json type coercion (Ryan Ofsky)
Pull request description:
Should probably add this change to 23.x as suggested by Luke https://github.com/bitcoin/bitcoin/issues/24457#issuecomment-1059825678. If settings like `prune` are added to `settings.json` in the future, it would be preferable for 23.x releases to respect the setting instead of crash.
---
Fix GUI startup crash reported by Rspigler in https://github.com/bitcoin/bitcoin/issues/24457 that happens if `settings.json` contains an integer value for any of the configuration options which GUI settings can currently clash with (-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
The fix is a one-line change in `ArgsManager::GetArg`. The rest of the PR just adds a regression test for the GUI and unit tests for ArgsManager::GetArg methods.
ACKs for top commit:
laanwj:
Code review ACK 5b1aae12ca
achow101:
ACK 5b1aae12ca
jonatack:
Code review ACK 5b1aae12ca
Tree-SHA512: 958991b4bead9b82a3879fdca0f8d6405e2a212b7c46cf356f078843a4f156e27fd75fc46e2013aa5159582ead06d343c1ed248d678b3e5bbd312f247e37894c
691d45fdc8 Add coinstatsindex_unclean_shutdown test (Ryan Ofsky)
eb6cc05da3 index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together (Martin Zumsande)
Pull request description:
Fixes#24076
Coinstatsindex currently writes the MuHash (`DB_MUHASH`) to disk in `CoinStatsIndex::WriteBlock()` and `CoinStatsIndex::ReverseBlock()`, but the best synced block is written in `BaseIndex::Commit()`. These are called at different points in time, both during the ThreadSync phase, and also after the initial sync is finished and validation callbacks (`BlockConnected()` vs `ChainStateFlushed()`) perform the syncing.
As a result, the index DB is temporarily in an inconsistent state, and if bitcoind is terminated uncleanly (so that there is no time to call `Commit()` by receiving an interrupt or by flushing the chainstate) this leads to problems:
On the next startup, `Init()` will read the best block and a MuHash that corresponds to a different (higher) block. Indexing will be picked up at the the best block processing some blocks again, but since MuHash is a rolling hash, it will process some utxos twice and the muhashes for all future blocks will be wrong, as was observed in #24076.
Fix this by always committing `DB_MUHASH` together with `DB_BEST_BLOCK`.
Note that the block data for the index is still written at different times, but this does not corrupt the index - at worst, these entries will be processed another time and overwritten after an unclean shutdown and restart.
ACKs for top commit:
ryanofsky:
Code review ACK 691d45fdc8. Only change since last review is adding test
fjahr:
ACK 691d45fdc8
Tree-SHA512: e1c3b5f06fa4baacd1b070abb0f8111fe2ea4a001ca8b8bf892e96597cf8b5d5ea10fa8fb837cfbf46648f052c742d912add4ce26d4406294fc5fc20809a0e1b
7a68fe4831 bitcoin-chainstate: Lock cs_main to UnloadBlockIndex (Carl Dong)
Pull request description:
This was introduced because of a silent merge conflict.
ACKs for top commit:
promag:
ACK 7a68fe4831
jonatack:
ACK 7a68fe4831
Tree-SHA512: 4c135efd68604452485a129e731675ff5917c157a70c77dd702211d9902c21b3b29380a881723f43ecba4762bc864b036881bb502b3b792e581565dcaa7a7ed4
7abd8b21ba doc: include wtxid in TransactionDescriptionString (brunoerg)
2d596bce6f doc: add wtxid info in release-notes (brunoerg)
a5b66738f1 test: add wtxid in expected_fields for wallet_basic (brunoerg)
e8c659a297 wallet: add wtxid in WalletTxToJSON (brunoerg)
7482b6f895 wallet: add GetWitnessHash() (brunoerg)
Pull request description:
This PR add `wtxid` in `WalletTxToJSON` which allows to return this field in `listsinceblock`, `listtransactions` and `gettransaction` (RPCs).
ACKs for top commit:
achow101:
re-ACK 7abd8b21ba
w0xlt:
crACK 7abd8b2
luke-jr:
re-utACK 7abd8b21ba
Tree-SHA512: f86f2dbb5e38e7b19932006121802f47b759d31bdbffe3263d1db464f6a3a30fddd68416f886a44f6d3a9fd570f7bd4f8d999737ad95c189e7ae5e8ec1ffbdaa
fa097d074b addrman: Log too low compat value (MarcoFalke)
Pull request description:
Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.
In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence.
ACKs for top commit:
mzumsande:
re-ACK fa097d074b
Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65
Fix GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 that happens if
settings.json contains an integer value for any of the configuration
options which GUI settings can currently clash with (-dbcache, -par,
-spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy,
-proxy, -onion, -onion, -lang, and -prune).
Fix is a one-line change in ArgsManager::GetArg.
Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.
Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
6c23c41561 refactor: Rewrite AddToBlockIndex with try_emplace (Carl Dong)
c05cf7aa1e style: Modernize range-based loops over m_block_index (Carl Dong)
c2a1655799 style-only: Use using instead of typedef for BlockMap (Carl Dong)
dd79dad175 refactor: Rewrite InsertBlockIndex with try_emplace (Carl Dong)
531dce0347 tests: Remove now-unnecessary manual Unload's (Carl Dong)
bec86ae326 blockstorage: Make m_block_index own CBlockIndex's (Carl Dong)
Pull request description:
Part of: #24303
Split off from: #22564
```
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
```
The second commit demonstrates how this makes calls to `Unload()` to satisfy the address sanitizer unnecessary.
ACKs for top commit:
ajtowns:
ACK 6c23c41561
MarcoFalke:
re-ACK 6c23c41561🎨
Tree-SHA512: 81b2b5119be27cc0f8a9457b11da60cc60930315d2a5be36be89fe253d32073ffe622348ff153114b9b3212197bddbc791810913a43811b33cc58e7162bd105b
1b20109b04 Squashed 'src/leveldb/' changes from f8ae182c1e..330dd6235f (MarcoFalke)
Pull request description:
A minor change to:
* Consistently use the same symbol names in the whole project.
* Fix compiling with C++20.
ACKs for top commit:
fanquake:
ACK fa0c32eb74
Tree-SHA512: b5d4540dd621cf4aa8caac811bae03bb74e502a31dbdda9354182e4caa39905550e62ad3cf8ea7d7f9bfc3e5120d119d34ab0f1e633716ec8089876037cbf192
ae9ceed3e2 validation, refactoring: remove ChainstateManager::Reset() (Jon Atack)
daad0093e3 validation: replace lock with annotation in UnloadBlockIndex() (Jon Atack)
Pull request description:
Thread safety refactoring seen in #24177:
- replace re-acquiring lock cs_main with a thread safety annotation in UnloadBlockIndex()
- remove ChainstateManager::Reset(), as it is currently unused (can be reintroduced in the test utilities if needed for unit testing)
ACKs for top commit:
laanwj:
Code review ACK ae9ceed3e2
vasild:
ACK ae9ceed3e2
klementtan:
crACK ae9ceed3e2
Tree-SHA512: cebb782572997cc2dda01590d6bb6c5e479e8202324d8b6ff459b814ce09e818b996c881736bfebd1b8bf4b6d7a0f79faf3ffea176a4699dd7d7429de2db2d13
60aa179d8f Use GetPathArg where possible (Pavol Rusnak)
5b946edd73 util, refactor: Use GetPathArg to read "-settings" value (Ryan Ofsky)
687e655ae2 util: Add GetPathArg default path argument (Ryan Ofsky)
Pull request description:
Improve `ArgsManager::GetPathArg` method added in recent PR #24265, so it is usable more places. This PR starts to use it for the `-settings` option. This can also be helpful for #24274 which is parsing more path options.
- Add `GetPathArg` default argument so it is less awkward to use to parse options that have default values.
- Fix `GetPathArg` negated argument handling. Return path{} not path{"0"} when path argument is negated.
- Add unit tests for default and negated cases
- Move `GetPathArg` method declaration next to `GetArg` declaration. The two methods are close substitutes for each, so this should help keep them consistent and make them more discoverable.
ACKs for top commit:
w0xlt:
Tested ACK 60aa179 on Ubuntu 21.10
hebasto:
re-ACK 60aa179d8f
Tree-SHA512: 3d24b885d8bbeef39ea5d0556e2f09b9e5f4a21179cef11cbbbc1b84da29c8fb66ba698889054ce28d80bc25926687654c8532ed46054bf5b2dd1837866bd1cd
faa329fd46 refactor: Release cs_main before MaybeSendFeefilter (MarcoFalke)
Pull request description:
There is no need for any lock to be held, because net processing is single threaded. So holding the validation lock cs_main for sending a feefilter is confusing and might even degrade blockchain-related RPC performance minimally.
ACKs for top commit:
ajtowns:
ACK faa329fd46 ; code review only
vasild:
ACK faa329fd46
Tree-SHA512: 3e7f9faff1631cc64c86fc1a354ada67617ad1e7a046625cc741f4711854eb41ca8aad5a51ef0d94ff65947b68dba8345c9f786b20ee0a8b7a2e8741cfced21f
6f2593dc23 gui, refactor: use std::chrono for formatDurationStr() helper (Jon Atack)
Pull request description:
Updates `formatDurationStr()` to use the `chrono` standard lib. No change in behavior.
ACKs for top commit:
RandyMcMillan:
tACK 6f2593dc23
shaavan:
ACK 6f2593dc23
w0xlt:
tACK 6f2593d on Ubuntu 21.10 Qt 5.15.2
promag:
Code review ACK 6f2593dc23.
Tree-SHA512: 61e9afdb1db779150df338e6af08727c34f69639add465c2f7003ff775d97dce3e78e78d325bc6dea5bc13f0fce9ef1c3506d13f1661a5e083e52bba8a32ba44
4828d53ecc Add (sorted)multi_a descriptors to doc/descriptors.md (Pieter Wuille)
b5f33ac1f8 Simplify wallet_taproot.py functional test (Pieter Wuille)
eb0667ea96 Add tests for (sorted)multi_a derivation/signing (Pieter Wuille)
c17c6aa08d Add signing support for (sorted)multi_a scripts (Pieter Wuille)
3eed6fca57 Add multi_a descriptor inference (Pieter Wuille)
79728c4a3d Add (sorted)multi_a descriptor and script derivation (Pieter Wuille)
25e95f9ff8 Merge/generalize IsValidMultisigKeyCount/GetMultisigKeyCount (Pieter Wuille)
Pull request description:
This adds a new `multi_a(k,key_1,key_2,...,key_n)` (and corresponding `sortedmulti_a`) descriptor for k-of-n policies inside `tr()`. Semantically it is very similar to the existing `multi()` descriptor, but with the following changes:
* The corresponding script is `<key1> OP_CHECKSIG <key2> OP_CHECKSIGADD <key3> OP_CHECKSIGADD ... <key_n> OP_CHECKSIGADD <k> OP_NUMEQUAL`, rather than the traditional `OP_CHECKMULTISIG`-based script, making it usable inside the `tr()` descriptor.
* The keys can optionally be specified in x-only notation.
* Both the number of keys and the threshold can be as high as 999; this is the limit due to the consensus stacksize=1000 limit
I expect that this functionality will later be replaced with a miniscript-based implementation, but I don't think it's necessary to wait for that.
Limitations:
* The wallet code will for not estimate witness size incorrectly for script path spends, which may result in a (dramatic) fee underpayment with large multi_a scripts.
* The multi_a script construction is (slightly) suboptimal for n-of-n (where a `<key1> OP_CHECKSIGVERIFY ... <key_n-1> OP_CHECKSIGVERIFY <key_n> OP_CHECKSIG` would be better). Such a construction is not included here.
ACKs for top commit:
achow101:
ACK 4828d53ecc
gruve-p:
ACK 4828d53ecc
sanket1729:
code review ACK 4828d53ecc
darosior:
Code review ACK 4828d53ecc
Tree-SHA512: 5dcd434b79585f0ff830f7d501d27df5e346f5749f47a3109ec309ebf2cbbad0e1da541eec654026d911ab67fd7cf7793fab0f765628d68d81b96ef2a4d234ce
bbbbeaf9c8 fuzz: Limit script_format to 100kB (MarcoFalke)
Pull request description:
The target is still one of the slowest ones, but doesn't seem incredibly important. Especially for sizes larger than the standard tx size.
Fix that by limiting the script size.
ACKs for top commit:
fanquake:
ACK bbbbeaf9c8
Tree-SHA512: b6cf7248753909ef2f21d8824f187e7c05732dd3b99619c0067f862f3c2b0f9a87779d4ddbbd3a7a4bae5c794280e2f0a223bf835d6bc6ccaba01817d69479a2
2c03cec2ff ci: Build bitcoin-chainstate (Carl Dong)
095aa6ca37 build: Add example bitcoin-chainstate executable (Carl Dong)
Pull request description:
Part of: #24303
This PR introduces an example/demo `bitcoin-chainstate` executable using said library which can print out information about a datadir and take in new blocks on stdin.
Please read the commit messages for more details.
-----
#### You may ask: WTF?! Why is `index/*.cpp`, etc. being linked in?
This PR is meant only to capture the state of dependencies in our consensus engine as of right now. There are many things to decouple from consensus, which will be done in subsequent PRs. Listing the files out right now in `bitcoin_chainstate_SOURCES` is purely to give us a clear picture of the task at hand, it is **not** to say that these dependencies _belongs_ there in any way.
### TODO
1. Clean up `bitcoin-chainstate.cpp`
It is quite ugly, with a lot of comments I've left for myself, I should clean it up to the best of my abilities (the ugliness of our init/shutdown might be the upper bound on cleanliness here...)
ACKs for top commit:
ajtowns:
ACK 2c03cec2ff
ryanofsky:
Code review ACK 2c03cec2ff. Just rebase, comments, formatting change since last review
MarcoFalke:
re-ACK 2c03cec2ff 🏔
Tree-SHA512: 86e7fb5718caa577df8abc8288c754f4a590650d974df9d2f6476c87ed25c70f923c4db651c6963f33498fc7a3a31f6692b9a75cbc996bf4888c5dac2f34a13b
Call fs::u8path to convert some UTF-8 string literals to paths, instead
of relying on implicit conversions. The implicit conversions incorrectly
decode const char* paths using the current windows codepage, instead of
treating them as UTF-8. This could cause test failures depending what
environment windows tests are run in.
Issue was reported by MarcoFalke <falke.marco@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/24306#discussion_r818566106
and harmonize them as follows
- s/outgoing/automatic outbound/
- s/Incoming/Inbound and manual/ (are not affected by this option.)
- s/only through network/only to network/
- s/this option. This option/this option. It/
- s/network types/networks/
and also pick up a few nits in doc/p2p-bad-ports.md
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
Add a new function `TestOnlyResetTimeData()` which would reset the
internal state used by `GetTimeOffset()`, `GetAdjustedTime()` and
`AddTimeData()`.
This is needed so that unit tests that call `AddTimeData()` can restore
the state in order not to confuse other tests that rely on it.
Currently `timedata_tests/addtimedata` is the only test that modifies
the state (via `AddTimeData()`) and also the only test that relies on
that state.
5d7f22595f Do not use `LocalTestingSetup` in getarg_tests test file. (Kiminuo)
Pull request description:
Avoid using a test fixture in getarg_tests for better readability. Change was implemented by _kiminuo_ and posted https://github.com/bitcoin/bitcoin/pull/24306#issuecomment-1036643216
ACKs for top commit:
kiminuo:
ACK 5d7f22595f
Tree-SHA512: 0fd98622010e6923e91c66447a1d0861bf344a65d86a313dff7d428c089b1740a25f699327f6ed4c163255f270bcbd4f7be962bb551862214f9b9e395d40df04
Let GetPathArg method be used more places for path arguments that have
default values, like "-settings" and BITCOIN_SETTINGS_FILENAME in the
next commit.
Also:
- Fix negated argument handling. Return path{} not path{"0"} when path
argument is negated.
- Add new tests for default and negated cases
- Move GetPathArg() method declaration next to GetArg() declarations.
The two methods are close substitutes for each other, so this should
help keep them consistent and make them more discoverable.
b7be28cac5 test: add combined CJDNS/I2P/localhost/onion eviction protection tests (Jon Atack)
0a1bb84770 test: add tests for inbound eviction protection of CJDNS peers (Jon Atack)
0c00c0c981 test: fix off-by-one logic in an eviction protection test (Jon Atack)
f7b8094d61 p2p: extend inbound eviction protection by network to CJDNS peers (Jon Atack)
Pull request description:
Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release.
ACKs for top commit:
laanwj:
Concept and code review ACK b7be28cac5
w0xlt:
tACK b7be28c
Tree-SHA512: 89ebdd217602e16ae14b9bd0d5a25fc09f9b2384c951f820bc0f5a6d8452bbc9042065db817d5d5296c0ad22988491a83fc5b9a611e660c40ebd4f03448c4061
c4d76c6faa tests: Tests for inactive HD chains (Andrew Chow)
8077862c5e wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow)
70134eb34f wallet: Properly set hd chain counters when loading (Andrew Chow)
961b9e4e40 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow)
0652ee73ec Add size check on meta.key_origin.path (Rob Fielding)
Pull request description:
Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.
This PR resolves this problem by adding memory only variables to `CHDChain` which track the highest known index. `TopUp` is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.
Note that because these variables are not persisted to disk (because `CHDChain`s for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.
Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in `CKeyMetadata.hdKeypath` to determine what indexes to derive.
ACKs for top commit:
laanwj:
Code review ACK c4d76c6faa
Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095
36ee76d1af net: remove unused CNetAddr::GetHash() (Vasil Dimov)
d0abce9a50 net: include the port when deciding a relay destination (Vasil Dimov)
2e38a0e686 net: add CServiceHash constructor so the caller can provide the salts (Vasil Dimov)
97208634b9 net: open p2p connections to nodes that listen on non-default ports (Vasil Dimov)
Pull request description:
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
ACKs for top commit:
laanwj:
Concept and light code review ACK 36ee76d1af
prayank23:
ACK 36ee76d1af
stickies-v:
tACK 36ee76d1a
jonatack:
ACK 36ee76d1af
glozow:
utACK 36ee76d1af
Tree-SHA512: 7f45ab7567c51c19fc50fabbaf84f0cc8883a8eef84272b76435c014c31d89144271d70dd387212cc1114213165d76b4d20a5ddb8dbc958fe7e74e6ddbd56d11
The default network reachability values are implicitly set
by this line in net.cpp:
static bool vfLimited[NET_MAX] GUARDED_BY(g_maplocalhost_mutex) = {};
This commit asserts that each network is reachable during
the first loop through them during bitcoind init.
0eea83a85e scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505db net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85e
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85e code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
c7376cc8d7 tests: Test upgrading wallet with privkeys disabled (Andrew Chow)
3d985d4f43 wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow)
Pull request description:
When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.
Fixes#23610
ACKs for top commit:
laanwj:
Code review ACK c7376cc8d7
benthecarman:
tACK c7376cc8d7 this fixed the issue for me
Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6
fa7991601c Fixup style of VerifyDB (MarcoFalke)
fa462ea787 Avoid implicit-integer-sign-change in VerifyLoadedChainstate (MarcoFalke)
Pull request description:
This happens when checking all blocks (`-1`).
To test:
```
./configure CC=clang CXX=clang++ --with-sanitizers=undefined,integer
make
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/rpc_blockchain.py
ACKs for top commit:
theStack:
Code-review ACK fa7991601c
brunoerg:
crACK fa7991601c
Tree-SHA512: bcbe6becf2fbedd21bbde83a544122e79465937346802039532143b2e4165784905a8852c0ccb088b964874df5e5550931fdde3629cbcee3ae237f2f63c43a8e
d80dc12097 net: Update hardcoded seeds for 23.x (laanwj)
9f27157894 contrib: make-seeds updates for 23.x (laanwj)
Pull request description:
Update hardcoded P2P network seeds for 23.x, and update the generation script and documentation as necessary
Tool output:
```
IPv4 IPv6 Onion Pass
469910 72944 0 Initial
469910 72944 0 Skip entries with invalid address
469910 72944 0 After removing duplicates
469909 72944 0 Skip entries from suspicious hosts
165760 65113 0 Enforce minimal number of blocks
160668 63183 0 Require service bit 1
4951 1376 0 Require minimum uptime
4406 1051 0 Require a known and recent user agent
4307 1031 0 Filter out hosts with multiple bitcoin ports
ERR: Could not resolve ASN for "2001:678:7dc:8::2": The DNS query name does not exist: 8.0.0.0.c.d.7.0.8.7.6.0.1.0.0.2.origin6.asn.cymru.com.
512 134 0 Look up ASNs and limit results per ASN and per net
```.
ACKs for top commit:
achow101:
ACK d80dc12097
jonatack:
ACK d80dc12097 reviewed the changes and ran the README steps
Tree-SHA512: c651b0501cc28d397cc0778eff6aed4273669082d6ef207ce58ce198b443be66532bf1e8d618ccae3ba671ae4cccfd9b4dd2dfebacc97f3c3bd4e9fa58a3d7a3
d41ed32153 p2p: Avoid InitError when downgrading peers.dat (junderw)
Pull request description:
fixes#24188 (also see https://github.com/bitcoin/bitcoin/pull/22762#issuecomment-951063826)
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded Bitcoin Core version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one.
ACKs for top commit:
prayank23:
reACK d41ed32153
kallewoof:
reACK d41ed32153
Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40
fixes#24188
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one, while creating a backup in peers.dat.bak.
- `m_assumed_chain_state_size` doesn't seem to need to be changed for mainnet.
- No change needed for testnet/signet.
Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>
7f3a6a9495 wallet: Add external-signer-support specific error message (Hennadii Stepanov)
Pull request description:
On master (5f44c5c428) an attempt to load an external signer wallet using Bitcoin Core compiled without external signer support fails with the following log messages:
```
2022-02-20T19:01:11Z [qt-walletctrl] Using SQLite Version 3.31.1
2022-02-20T19:01:11Z [qt-walletctrl] Using wallet /home/hebasto/.bitcoin/testnet3/wallets/coldcard-0220
2022-02-20T19:01:11Z [qt-walletctrl] init message: Loading wallet…
2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Error: External signer wallet being loaded without external signer support compiled
2022-02-20T19:01:11Z [qt-walletctrl] [coldcard-0220] Releasing wallet
```
While log messages are good, a message in the GUI window is completely misleading:
This PR fixes this issue:
ACKs for top commit:
achow101:
ACK 7f3a6a9495
kristapsk:
ACK 7f3a6a9495
brunoerg:
crACK 7f3a6a9495
Tree-SHA512: a4842751c0ca8a37ccc3ea00503678f6b712a7f53d6cbdc07ce02dcb85ca8a94890d1c2da20307be043faa347747abeba29185c88ba12edd5253bfca56531585
fad7ddf9e3 test: Run symlink regression tests on Windows (MarcoFalke)
Pull request description:
Seems odd to add tests, but not run them on the platform that needs them most.
ACKs for top commit:
laanwj:
Code review ACK fad7ddf9e3
ryanofsky:
Code review ACK fad7ddf9e3, just removing new test. Would be nice if the test could be added later, of course.
Tree-SHA512: 64b235967a38c2eb90657e8d7a0447bcc8ce81d1b75a275b6c48bd42efd9ea7e7939257e484f297ee84598def3738eaeb289561aeba1dd6a99b258d389995139
9999f891d1 bench: Avoid deprecated use of volatile += (MarcoFalke)
Pull request description:
Deprecated in C++20 according to https://eel.is/c++draft/expr.ass#6 .
```
bench/examples.cpp:16:13: warning: compound assignment with ‘volatile’-qualified left operand is deprecated [-Wvolatile]
16 | sum += sin(d);
| ~~~~^~~~~~~~~
```
While C++20 is currently unsupported, I don't see any downside to a minor fixup to an example benchmark. This will also make a hypothetical C++20 patch smaller.
ACKs for top commit:
fanquake:
ACK 9999f891d1
Tree-SHA512: ca7d660fa8eba347a4648408a8b97a0ecb8263a825da7abd59129d783058102581e05b273667989f95480436a66d5384bd1e92d9ae79408f5b30e2178935cc38
fafc4eb363 test: Fix Wambiguous-reversed-operator compiler warnings (MarcoFalke)
Pull request description:
Add a missing const to avoid the C++20 clang **compiler warning**:
```
test/fuzz/addrman.cpp:325:22: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'AddrManDeterministic' and 'AddrManDeterministic') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator]
assert(addr_man1 == addr_man2);
~~~~~~~~~ ^ ~~~~~~~~~
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
test/fuzz/addrman.cpp:140:10: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
bool operator==(const AddrManDeterministic& other)
^
1 error generated.
```
This patch also fixes the **compile error** if the first operand is `const`:
```
test/fuzz/addrman.cpp:326:23: error: invalid operands to binary expression ('const AddrManDeterministic' and 'AddrManDeterministic')
assert(addr_man_1 == addr_man2);
~~~~~~~~~~ ^ ~~~~~~~~~
/usr/include/assert.h:90:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
test/fuzz/addrman.cpp:140:10: note: candidate function not viable: 'this' argument has type 'const AddrManDeterministic', but method is not marked const
bool operator==(const AddrManDeterministic& other)
^
1 error generated.
ACKs for top commit:
hebasto:
ACK fafc4eb363, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 92cd62ae06ee1393a6dc2ea6f3f553595a8f8d66f51592d231b42122bfb71ed4801a016daafc85360040339c5ae59b76888265cec37449c4688d6c7768f4567e
Given the low possibility of a reorg reverting the segwit soft fork,
there is no need to check whether segwit is active here. Also,
TestBlockValidity is run on the block template after it has been
created.
Instead of having CBlockIndex's live on the heap, which requires manual
memory management, have them be owned by m_block_index. This means that
they will live and die with BlockManager.
A change to BlockManager::LookupBlockIndex:
- Previously, it was a const member function returning a non-const CBlockIndex*
- Now, there's are const and non-const versions of
BlockManager::LookupBlockIndex returning a CBlockIndex with the same
const-ness as the member function:
(e.g. const CBlockIndex* LookupBlockIndex(...) const)
See next commit for some weirdness that this eliminates.
The range based for-loops are modernize (using auto + destructuring) in
a future commit.
Consolidate to outputting the licensing info when we pass -version to a binary,
i.e bitcoind -version:
```bash
itcoin Core version v22.99.0-fc1f355913f6-dirty
Copyright (C) 2009-2022 The Bitcoin Core developers
Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.
This is experimental software.
Distributed under the MIT software license, see the accompanying file COPYING
or <https://opensource.org/licenses/MIT>
```
48742693ac Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd434 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693ac
hebasto:
re-ACK 48742693ac, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
e50a9be154 Remove outdated comment on CFeeRate (Murch)
Pull request description:
This comment described how the constructor of CFeeRate was previously indirectly used to parse fee rate arguments from RPCs. The command line input was actually in sat/vB but due to the use of AmountFromValue() it got converted to BTC/vB which then got rectified in the constructor by creating a CFeeRate from that given value and COIN as the transaction size. Since this usage pattern was removed from the codebase some months ago, the comment is now obsolete.
ACKs for top commit:
michaelfolkson:
ACK e50a9be154
jonatack:
ACK e50a9be154
Tree-SHA512: f17bf0baeeca85a5c7883edadd407da845f6e3af1c949e93116bd67c02e601682a5f7f1ab2497172472e3acf1c4e3c234b01161a77e7d7f028e3551da34777f0
e7fc50681e qt: Override BitcoinApplication::event() to handle QEvent::Quit (Hennadii Stepanov)
Pull request description:
bitcoin-core/gui#336 introduced a regression when termination requests from a platform are not handled properly.
This PR fixes this regression. On macOS shutdown after clicking "Quit" in Dock icon menu, and during logout works again.
Fixesbitcoin-core/gui#545.
ACKs for top commit:
RandyMcMillan:
tACK e7fc50681e
Sjors:
tACK e7fc50681e (rebased on master) indeed fixes the crash described in #545
promag:
Tested ACK e7fc50681e on macOS 10.15 with Qt 5.15.2.
Tree-SHA512: 236a483dc0828f22999469e133b8ac9f0b6267ec2a27004c3ebaa967689ddb972ea1fa90c1dd41f3bff3d17bf571a707babcef53bd79fd711fda98cfbf120131
ce690847b6 cli: describe quality/recency filtering in -addrinfo (Jon Atack)
7c975614c0 rpc: describe quality/recency filtering in getnodeaddresses (Jon Atack)
Pull request description:
Addresses #24278.
```
$ bitcoin-cli help getnodeaddresses
getnodeaddresses ( count "network" )
Return known addresses, after filtering for quality and recency.
These can potentially be used to find new peers in the network.
The total number of addresses known to the node may be higher.
```
```
$ bitcoin-cli -help | grep -A3 addrinfo
-addrinfo
Get the number of addresses known to the node, per network and total,
after filtering for quality and recency. The total number of
addresses known to the node may be higher.
```
ACKs for top commit:
mzumsande:
Thanks, Code Review ACK ce690847b6
prayank23:
reACK ce690847b6
Tree-SHA512: 82d23b15e64a99411eb8e70d7267a1b4f23182fabe072e824277569d9677e392b466be63f00e3d157d7db94bbe032d53f12ad4ab30b55b7b8a629c37d80d1d8c
c821ab8be8 Use `GetAllOutputTypes` in `getblock` RPC function (Kiminuo)
d970a85d33 Move `GetAllOutputTypes` function from `rpc/rawtransaction.cpp` to `rpc/util.{h|cpp}` (Kiminuo)
Pull request description:
This PR attempts to replicate 0ccf9b2e55/src/rpc/rawtransaction.cpp (L547) to one other place (at the moment) so that users have better idea what RPC methods can actually return.
I created this PR as a follow-up to the idea mentioned here https://github.com/bitcoin/bitcoin/pull/23320#discussion_r732458112 (resolved).
ACKs for top commit:
kristapsk:
re-ACK c821ab8be8
Tree-SHA512: 5ff66a41ad7c43ec769f4a99933d2d070feea7c617286d94b6f9bfa1a2547a42211915778210a89074ad4b14d99f34852cc6871efed5e6f1e2ffedd40d669386
fa30e62cc6 doc: Rework generate* doc (MarcoFalke)
Pull request description:
Hide the test-only calls and clarify the short description
ACKs for top commit:
0xB10C:
reACK fa30e62cc6. changes since fa3bb584dcc742a767b2141cd7324877e3cf5302 are: dropping the `immediately` + formatting the touched line and a rebase
Tree-SHA512: 07439f39660bbf144c2cc406b6010b64dcdd27150d78654fe04a36a982a519f837a0cf0f030c9f30af69c451ccf7a3b7287a275637aa81904c202029b9efc661
0683f377e1 Add tr() descriptor unit tests (Pieter Wuille)
4b2e31a7ae Bugfix: make ToPrivateString work with x-only keys (Pieter Wuille)
18ad54c3b2 Bugfix: set x-only flag when inferring pk() inside tr() (Pieter Wuille)
Pull request description:
This fixes two bugs in the current logic for `tr()` descriptors:
* ToPrivateString does not always work, because the provided private key may mismatch the parity of the x-only public key.
* The descriptors inferred for `pk()` inside `tr()` have the wrong x-only flag, leading to such descriptors generating the wrong scriptPubKey (roundtripping through ToString does fix it however, so this seems unobservable in the current code).
These were discovered while adding unit tests to descriptor_tests that cover various aspects of `tr()` descriptors, which are now also added here.
ACKs for top commit:
achow101:
ACK 0683f377e1
instagibbs:
ACK 0683f377e1
jonatack:
Code review ACK 0683f377e1
Tree-SHA512: fc0e11b45da53054a108effff2029d67b64e508b160a6e22e00c98b506c39ec12ccc95afd21ea68a6c691eb62930afc7af18908f2fa3a954d102afdc67bc355a
6981de4435 doc: fix wording of alertnotify (willcl-ark)
Pull request description:
The documentation of the `alertnotify` startup option no longer matches the implementation.
Currently the alert is only triggered by `DoWarning` (as part of `CChainstate::UpdateTip` when blocks containing unknown versionbits are detected on the network, indicating that there may be an upcoming softfork which you don't know about), but not when we see a "really long fork":
2825c41a61/src/validation.cpp (L2418-L2433)
I think it would be desirable in a follow-up PR to implement the logic to alert on a (really) long fork, but not to alert for "partition detection" (abnormally slow/fast blocks). `PartitionChecker` code was removed in ab8be98fdb
ACKs for top commit:
josibake:
ACK 6981de4435
achow101:
ACK 6981de4435
Tree-SHA512: ea124f53ca1db803ba93d649f4bc983484c47fb5fe7fa61a8eb32fcbc7425f67d8578e66a6ba70202e13868fe8add0103306dede3b1edd1d3261ffb9c1042b87
62cc138ecb Rename wallet-tool to bitcoin-wallet in code comment (Kristaps Kaupe)
0db3ad3ba4 Mention -signet in bitcoin-wallet help output (Kristaps Kaupe)
Pull request description:
* Mention `-signet` in sentence where there is already `-testnet/-signet` in help output.
* Rename `wallet-tool` to `bitcoin-wallet` in single remaining place in code comments (was already done in #17648 at other places).
ACKs for top commit:
RandyMcMillan:
tACK 62cc138ecb
Tree-SHA512: c5df7811b8200f61943908dcf3b2b788fe991bf00bef28f069ab8784924556ffd5d86fc0ba2ad0b3c3f9be2ba73a34bc67059d7c057bba646c1801ffa3cb2070
fa1b89a6bd scripted-diff: Rename nReadPos to m_read_pos in streams.h (MarcoFalke)
fa56c79df9 Make CDataStream work properly on 64-bit systems (MarcoFalke)
fab02f7991 streams: Fix read-past-the-end and integer overflows (MarcoFalke)
Pull request description:
This is a follow-up to commit e26b62093a with the following fixes:
* Fix unsigned integer overflow in `ignore()`, when `nReadPos` wraps.
* Fix unsigned integer overflow in `read()`, when `nReadPos` wraps.
* Fix read-past-the-end in `read()`, when `nReadPos` wraps.
This shouldn't be remote-exploitable, because it requires a stream of more than 1GB of size. However, it might be exploitable if the attacker controls the datadir (I haven't checked).
A unit test for the overflow in `ignore()` looks like following. It is left as an excercise to the reader to replace `foo.ignore(7)` with the appropriate call to `read()` to reproduce the overflow and read-error in `read()`.
```diff
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index 922fd8e513..ec6ea93919 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -534,6 +534,20 @@ BOOST_AUTO_TEST_CASE(ccoins_serialization)
} catch (const std::ios_base::failure&) {
}
+ CDataStream foo{0, 0};
+ auto size{std::numeric_limits<uint32_t>::max()};
+ foo.resize(size);
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ foo.ignore(std::numeric_limits<int32_t>::max());
+ size -= std::numeric_limits<int32_t>::max();
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ foo.ignore(std::numeric_limits<int32_t>::max());
+ size -= std::numeric_limits<int32_t>::max();
+ BOOST_CHECK_EQUAL(foo.size(), size);
+ BOOST_CHECK_EQUAL(foo.size(), 1);
+ foo.ignore(7); // Should overflow, as the size is only 1
+ BOOST_CHECK_EQUAL(foo.size(), uint32_t(1 - 7));
+
// Very large scriptPubKey (3*10^9 bytes) past the end of the stream
CDataStream tmp(SER_DISK, CLIENT_VERSION);
uint64_t x = 3000000000ULL;
```
ACKs for top commit:
klementtan:
Code Review ACK fa1b89a6bd:
Tree-SHA512: 67f0a1baafe88eaf1dc844ac55b638d5cf168a18c945e3bf7a2cb03c9a5976674a8e3af2487d8a2c3eae21e5c0e7a519c8b16ee7f104934442e2769d100660e9
fa8dad0e07 rpc: Fix implicit-integer-sign-change in verifychain (MarcoFalke)
Pull request description:
It doesn't really make sense to treat `DEFAULT_CHECKLEVEL` as unsigned as long as `VerifyDB` accepts a signed integer.
Making it signed also avoids a cast round trip from signed->unsigned->signed in the RPC.
ACKs for top commit:
luke-jr:
utACK fa8dad0e07
theStack:
Code-review ACK fa8dad0e07
Tree-SHA512: 75499dbe4ace2962792e5fbec7defb10c25fdbbfde951d5e542a91daa880cc50395da0287173e2c84a28e18267c74af7b44b9f38ce364bcb0216c402f65b7641
820c03aff5 index: check muhash is in sync on coinstatsindex launch (Fabian Jahr)
38ed58b850 index: remove txindex references from base index (Fabian Jahr)
Pull request description:
This change lets the `coinstatsindex` fail loudly in case the internal `muhash` state differs from the last finalized output saved on disk, which would indicate that the `muhash` state somehow got out of sync. This should generally not happen since both are written to disk in a batch but #24076 seems to indicate that the might still be an issue.
Since #24076 so far can not be reproduced reliably, the issue should not be closed yet. Further investigation and testing needs to be done.
ACKs for top commit:
Sjors:
re-ACK 820c03aff5
mzumsande:
re-ACK 820c03aff5
ryanofsky:
Code review ACK 820c03aff5. Good to catch the error earlier
Tree-SHA512: 3c985d7152698d25bad95d4ad512ff87dff13fabef790589c5a6cf93ca4251ad599e12feb7251a084503e2a213b022eaacfbaaa601464114ad372b029f64f204
799968e8b3 tracing: misc follow-ups to 22902 (0xb10c)
36a6584703 tracing: correctly scope utxocache:flush tracepoint (Arnab Sen)
Pull request description:
This PR is a follow-up to the [#22902](https://github.com/bitcoin/bitcoin/pull/22902).
Previously, the tracepoint `utxocache:flush` was called, even when it was not flushing. So, the tracepoint is now scoped to write only when coins cache to disk.
ACKs for top commit:
0xB10C:
ACK 799968e8b3
Tree-SHA512: ebb096cbf991c551c81e4339821f10d9768c14cf3d8cb14d0ad851acff5980962228a1c746914c6aba3bdb27e8be53b33349c41efe8bab5542f639916e437b5f
d4b3483cec Primitives: Correct CTransaction deserialization docstring (TheCharlatan)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/8589 CTxWitness was removed and instead replaced with CScriptWitness inside each CTxIn.
ACKs for top commit:
w0xlt:
ACK d4b3483
Tree-SHA512: 02bb73e8a7d1fc449e4776a162009261baecc573837fade74ad7d76b3cd63200424e02fd0abd000c63706072f2ab3c95d3053139495b81347463f43e56192ca9
a4da16fbd4 Improve -netinfo help based on feedback from users and devs (Jon Atack)
Pull request description:
Clarify which networks are displayed by the peer counts table (*reachable* networks; follow-up to #23324) in response to questions received over the past months, and a few other improvements.
ACKs for top commit:
laanwj:
Code review ACK a4da16fbd4
w0xlt:
ACK a4da16f
kristapsk:
utACK a4da16fbd4
Tree-SHA512: e6522c08421aa7f10d50723156d0a8fc5ec82cad2f0bd931bbec603077fcd4921c6505ef743d57386fba81c95dcfc77df75abf3378319886368e4ae33f9a6d73
fae3f17823 fuzz: Split script formatting from script fuzz target (MarcoFalke)
Pull request description:
This is a follow-up to commit 9237bdaac1.
The target was improved a bit, but is still taking enormously long. See for example 4096 seconds in https://cirrus-ci.com/task/5153886888525824?logs=ci#L4451.
Most of the time is spent formatting the script. See the flamegraph: 
Thus, I suggest to split up the formatting into a new target. This will:
* Allow more fuzz cycles in the `script` target when exploring the search space with the fuzz engine
* Hopefully allow to reduce the fuzz inputs in `qa-assets` without losing coverage
ACKs for top commit:
fanquake:
ACK fae3f17823
Tree-SHA512: f86154b23019b7721e5dd10f54d11f4f7603d280471a396cb5256f4c460f48333318a60efe8b77fa8749a4abc67ad2631211b766fde5da70ded9fab8f904747b
b223c3c21e test: Add functional test for symlinked blocks directory (laanwj)
ddb75c2e87 test: Add fs_tests/create_directories unit test (Hennadii Stepanov)
1f46b6e46e util: Work around libstdc++ create_directories issue (laanwj)
Pull request description:
Work around libstdc++ issue [PR101510] with create_directories where the leaf already exists as a symlink. Fixes#24257, introduced by the switch to `std::filesystem`. It is meant to be more thorough than #24266, which worked around one instance of the problem.
The issue was [fixed upstream](https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc), but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
ACKs for top commit:
jonatack:
re-ACK b223c3c21e per `git range-diff df08250 67019cd b223c3c`
hebasto:
re-ACK b223c3c21e
w0xlt:
re-ACK b223c3c
vasild:
ACK b223c3c21e
Tree-SHA512: 028321717c8b10d16185c3711b35da6b05fb7aa31cee1c8c7e754e92bf5a0b02719a3785cd0f6f8bf052b3bd759f644af212320672baabc9e44e0b93ba464abc
eb8b22d517 block_connected: re-use previous GetTimeMicros (William Casarin)
80e1c55687 block_connected: don't serialize block hash twice (William Casarin)
Pull request description:
In the validation:block_connected tracepoint, we call block->GetHash(), which
ends up calling CBlockHeader::GetHash(), executing around 8000 serialization
instructions. We don't need to do this extra work, because block->GetHash() is
already called further up in the function. Let's save that value as a local
variable and re-use it in our tracepoint so there is no unnecessary tracepoint
overhead.
Shave off an extra 100 or so instructions from the validation:block_connected
tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down
to 54 instructions. Still high, but much better than the previous ~154 and
8000 instructions which it was originally.
Signed-off-by: William Casarin <jb55@jb55.com>
ACKs for top commit:
0xB10C:
ACK eb8b22d517
laanwj:
Code review ACK eb8b22d517
theStack:
re-ACK eb8b22d517
Tree-SHA512: 92ae585e487554e0f73042a8abaa239f630502c1d198e010bd7c1de252d882bccb627bbf0e4faec09c1253e782b145bcf153f9fee78cdb8456188044a96f8267
Work around libstdc++ issue [PR101510] with create_directories where the
leaf already exists as a symlink. Fixes#24257, introduced by the switch
to `std::filesystem`. It is meant to be more thorough than #24266, which
only worked around one instance of the problem.
The issue was fixed upstream in
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc,
but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
dc01cbc538 test: Add fs_tests/rename unit test (Hennadii Stepanov)
d4999d40b9 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov)
Pull request description:
Unfortunately, bitcoin/bitcoin#24308 introduced a [regression](https://github.com/bitcoin/bitcoin/pull/24308#issuecomment-1037259386) for mingw builds.
The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior
> If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_...
fails with the "File exists" error.
This PR reverts back the `MoveFileExW` call, and adds the [suggested](https://github.com/bitcoin/bitcoin/pull/24308#pullrequestreview-878832906) unit test.
ACKs for top commit:
vasild:
ACK dc01cbc538
Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
f485a07454 Add missing thread safety lock assertions in validation.h (Jon Atack)
37af8a20cf Add missing thread safety lock assertions in validation.cpp (Jon Atack)
Pull request description:
A number of functions in validation.{h,cpp} have a thread safety lock annotation in the declaration but are missing the corresponding run-time lock assertion in the definition.
ACKs for top commit:
hebasto:
re-ACK f485a07454, only suggested change since my [previous](https://github.com/bitcoin/bitcoin/pull/24177#pullrequestreview-877810465) review.
vasild:
ACK f485a07454
Tree-SHA512: c86c0c0e8fe6ec7ae9ed9890f1dd7d042aa482ecf99feb6679a670aa004f6e9a99f7bc047205a34968fab7f1f841898c59b48c3ed6245c166e3b5abbf0867445
bfcd60f5d5 test: activate all index types in feature_init.py (Martin Zumsande)
0243907fae index: Don't commit without valid m_best_block_index (Martin Zumsande)
Pull request description:
When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip ([code](06b6369766/src/chain.cpp (L31-L32))), and saves it to the index DB.
On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.
This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.
This is another small bug found by `feature_init.py` - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.
ACKs for top commit:
fjahr:
reACK bfcd60f5d5
shaavan:
reACK bfcd60f5d5
Tree-SHA512: 8e2bac0fc40cde209518a9e59b597ae0a5a875a2a90898673987c91733718d40e528dada942bf552b58bc021bf46e59da2d0cc5a61045f48f9bae2b1baf6033b
fa455975e5 util: Add missing unlinkat to syscall sandbox (MarcoFalke)
Pull request description:
This will be needed for g++-12 (after libstdc++6 12-20220206).
Steps to reproduce:
```
gdb --args ./src/bitcoind -sandbox=log-and-abort -regtest
./src/bitcoin-cli -regtest -named createwallet wallet_name=a descriptors=false
./src/bitcoin-cli -regtest stop
```
BT:
```
Thread 1 "b-shutoff" received signal SIGSYS, Bad system call.
0x00007ffff79564f7 in unlinkat () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) bt
#0 0x00007ffff79564f7 in unlinkat () from /lib/x86_64-linux-gnu/libc.so.6
#1 0x00007ffff7cc7335 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#2 0x00007ffff7cc94e3 in std::filesystem::remove_all(std::filesystem::__cxx11::path const&) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3 0x00005555559d4918 in wallet::BerkeleyEnvironment::Flush (this=0x7fffc4005160, fShutdown=<optimized out>) at /usr/include/c++/12/bits/fs_path.h:595
#4 0x000055555592c058 in wallet::StopWallets (context=...) at /usr/include/c++/12/bits/shared_ptr_base.h:1665
#5 0x00005555556617ca in Shutdown (node=...) at ./src/init.cpp:293
#6 0x000055555563ada6 in AppInit (argv=<optimized out>, argc=<optimized out>, node=...) at ./src/bitcoind.cpp:249
#7 main (argc=<optimized out>, argv=<optimized out>) at ./src/bitcoind.cpp:273
ACKs for top commit:
laanwj:
Code review ACK fa455975e5
Tree-SHA512: e80a38828f8656040954c9befa2d1c9d5170e204dc09c61031633349897f51ccd85cc5c99a089c4726d7f5237875cd9ed3fa8ef864cd6c1c8a2b8250b392d57f
f730bd7d58 scripted-diff: Rename ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)
5d7666b151 qt: Revert 7fa91e8312 partially (Hennadii Stepanov)
89c277a6fc qt: Delay shutdown while a modal dialog is active (Hennadii Stepanov)
8c0eb80f41 qt: Disable tray icon menu when a modal dialog is active (Hennadii Stepanov)
92427354dd qt, refactor: Use local QAction instances for the tray icon menu (Hennadii Stepanov)
58e16035c1 qt, refactor: Drop BitcoinGUI::{send,receive}CoinsMenuAction members (Hennadii Stepanov)
fd667e73cd qt: Make show_hide_action dependent on the main window actual state (Hennadii Stepanov)
ee151d0327 qt: Drop BitcoinGUI::toggleHideAction member (Hennadii Stepanov)
78189daac8 qt, refactor: Fill up trayIconMenu before connections (Hennadii Stepanov)
66afa286e5 qt, refactor: Replace BitcoinGUI::trayIconActivated with a lambda (Hennadii Stepanov)
c3ca8364b2 qt, refactor: Replace BitcoinGUI::macosDockIconActivated with a lambda (Hennadii Stepanov)
Pull request description:
As pointed in bitcoin/bitcoin#23790 a regression in wallet unlock was introduced in bitcoin-core/gui#336 when a synchronous `AskPassphraseDialog` has been replaced with an asynchronous one.
This PR reverts a call back to a synchronous mode.
To make synchronous dialogs behave nice during shutdown some additional changes were made.
Please note that disabling the tray icon menu when a modal dialog is active is useful itself as on master (4ad59042b3) it is possible to switch to the "Receive" tab while the GUI is waiting for a password for the "Send" tab:
This is confusing and must be avoided.
Fixesbitcoin/bitcoin#23790.
ACKs for top commit:
prayank23:
tACK f730bd7d58
Tree-SHA512: 2b68275754190e4a9831b96e882d3c5b005e03909aeb6f2c5846da07199bb3efbb74ce87a9d25bb139f643c43d377a2051b221d553281fa5aefdd3181a58077f
This comment described how the constructor of CFeeRate was previously
indirectly used to parse fee rate arguments from RPCs. The command line
input was actually in sat/vB but due to the use of AmountFromValue() it
got converted to BTC/vB. In the constructor this could be rectified by
creating a CFeeRate from that given value (in BTC/vB) and COIN as the
transaction size, turning the input effectively to sat/vB. Since this
usage pattern was removed from the codebase some months ago, the comment
is now obsolete.
Also:
• Use vsize and vbyte instead of size and byte
...to make sure that the linker errors that arise from coupling
regressions are caught by CI.
Adding to the "no wallet" ci job as suggested by MarcoFalke.
The bitcoin-chainstate executable serves to surface the dependencies
required by a program wishing to use Bitcoin Core's consensus engine as
it is right now.
More broadly, the _SOURCES list serves as a guiding "North Star" for the
libbitcoinkernel project: as we decouple more and more modules of the
codebase from our consensus engine, this _SOURCES list will grow shorter
and shorter. One day, only what is critical to our consensus engine will
remain. Right now, it's "the minimal list of files to link in to even
use our consensus engine".
[META] In a future commit the libbitcoinkernel library will be extracted
from bitcoin-chainstate, and the libbitcoinkernel library's
_SOURCES list will be the list that we aim to shrink.
34d0e07e92 Test that OP_1-OP_16 (but not lower/higher) start witness programs (Pieter Wuille)
Pull request description:
Cherry-picks one of the commits adding test coverage from #13062. As [pointed out by aj](https://github.com/bitcoin/bitcoin/pull/13062/files#r492723037):
> could move the test additions to the first commit, since they're testing things that are already true
Pull the additional test code into master earlier.
ACKs for top commit:
laanwj:
Code review ACK 34d0e07e92
Tree-SHA512: ff0ab2a54613ea6e8246b443363b362dd41b5e464faba4d11be6003aa6588a626cf56e142a3b94465cd37dd3ac4debea08455db96bade336171b6c30ea894950
fa6065661a refactor: Avoid unsigned integer overflow in core_write (MarcoFalke)
Pull request description:
Also, I find the new code a bit easier to understand.
ACKs for top commit:
shaavan:
Code Review ACK fa6065661a
Tree-SHA512: cd751e3b4dc97ef525eb8be8d0a49e9629389cb114df18d59a06e05388822af2939078e937f01494e6b317d601743b1a433ba47aa40c4dc602372d1f0fd0dc11
-BEGIN VERIFY SCRIPT-
sed -i 's/ShowModalDialogAndDeleteOnClose/ShowModalDialogAsynchronously/' -- $(git grep -l -e "ShowModalDialogAndDeleteOnClose")
-END VERIFY SCRIPT-
It is important to highlight that a modal dialog is showed
asynchronously as there are cases when the synchronous QDialog::exec()
is required.
The AskPassphraseDialog modal dialog must be synchronous here as
expected in the WalletModel::requestUnlock() function.
Fixed an introduced regression.
b75f4c89ec RPC: Return external_signer in getwalletinfo (Kristaps Kaupe)
Pull request description:
Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet.
ACKs for top commit:
S3RK:
utACK b75f4c89ec
achow101:
ACK b75f4c89ec
prayank23:
utACK b75f4c89ec
brunoerg:
utACK b75f4c89ec
Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
ee822d85d6 util: use stronger-guarantee rename method (Vasil Dimov)
Pull request description:
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
This is a rebase of #20435 by vasild.
ACKs for top commit:
MarcoFalke:
review ACK ee822d85d6
hebasto:
Approach ACK ee822d85d6.
vasild:
ACK ee822d85d6
Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a
In `PeerManagerImpl::RelayAddress()` we used just the hash of the
address that is being relayed to decide where to relay it to. Include
the port in that hash, so that e.g. `1.1.1.1:5555` and `1.1.1.1:6666`
get relayed to different peers. Those are two different, distinct
services after all.
This new constructor will be useful if we just want to hash a `CService`
object without the two `GetRand()` calls (in `RelayAddress()` in a
subsequent commit).
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
0c49e52b22 build: remove unneeded getentropy detection (HAVE_GETENTROPY) (Sebastian Falbesoner)
5cd15ffdce random: use arc4random on OpenBSD (Sebastian Falbesoner)
Pull request description:
Inspired by a discussion on obtaining randomness on various OSes in a secp256k1 PR (https://github.com/bitcoin-core/secp256k1/pull/748#discussion_r524605472, see also https://bitcoincore.reviews/libsecp256k1-748), I think it makes sense to follow best practices and use `arc4random_buf` rather than `getentropy` on OpenBSD in our random module.
The [getentropy(2) man page](https://man.openbsd.org/getentropy.2) states:
```
getentropy() is not intended for regular code; please use the
arc4random(3) family of functions instead.
```
The [arc4random(3) man page](https://man.openbsd.org/arc4random.3) states:
```
Use of these functions is encouraged for almost all random number
consumption because the other interfaces are deficient in either quality,
portability, standardization, or availability.
```
On the linked PR discussion worries about using RC4 internally has been expressed (see https://security.stackexchange.com/questions/85601/is-arc4random-secure-enough/172905#172905), but this would only affect users of OpenBSD <5.5, using a version that was released more than 8 years ago.
ACKs for top commit:
laanwj:
Tested ACK 0c49e52b22
Tree-SHA512: b5ed3d0718962c5a3839db9a28f93d08a0ac93094cc664f83bc4cf1cfad25049e6240b7b81fe06b71e6a3a0ca24a2c337eab088abec5470ad014e10c04fdb216
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
fac9fe5d05 Fix unintended unsigned integer overflow in strencodings (MarcoFalke)
Pull request description:
This fixes two issues for strings that start with a colon and only have one colon:
* `fMultiColon` is incorrectly set to `true`
* There is an unsigned integer overflow `colon - 1` (`0 - 1`)
Neither issue matters, as the result is discarded. Though, it makes sense to still fix the issue for clarity and to avoid sanitizer issues in the function.
ACKs for top commit:
laanwj:
Code review ACK fac9fe5d05
shaavan:
Code Review ACK fac9fe5d05
Tree-SHA512: e71c21a0b617abf241e561ce6b90b963e2d5e2f77bd9547ce47209a1a94b454384391f86ef5d35fedd4f4df19add3896bb3d61fed396ebba8e864e3eeb75ed59
fa2f7d0059 fuzz: Avoid unsigned integer overflow in FormatParagraph (MarcoFalke)
Pull request description:
`FormatParagraph` is only ever called with compile time constant arguments, so I don't see the need for fuzzing it.
Though, keep it for now, but avoid the unsigned integer overflow with this patch.
ACKs for top commit:
laanwj:
Code review ACK fa2f7d0059
Tree-SHA512: 01fc64a9ef73c183921ca1b0cd8db9514c0a242e3acf215a3393f383ae129e01625ebb16eaf9cb86370eda62d0145c3dcf8f62e40edf5958abc1f777c5687280
fa1b227a72 Remove broken and unused CDataStream methods (MarcoFalke)
faee5f8dc2 test: Create fresh CDataStream each time (MarcoFalke)
fa71114926 test: Inline expected_xor (MarcoFalke)
Pull request description:
The `insert` and `erase` methods have many issues:
* They are unused
* They are confusing and hard to read, as they implement "special cases" for optimization, that isn't needed
* They are broken (See https://github.com/bitcoin/bitcoin/pull/24231)
* Fixing them leads to mingw compile errors (See https://github.com/bitcoin/bitcoin/pull/24231#issuecomment-1029286985)
Fix all issues by removing them
ACKs for top commit:
laanwj:
Code review ACK fa1b227a72
Tree-SHA512: 9d9e5d42e6ffc5ae82bdb67cfb5b50b45977ae674acee6ff99092560aebf2fc7e4584ded614e190db0663226fa198e34350517cd7ee57d518de22e9568bc349a
fac62056b5 Fix integer sanitizer suppressions in validation.cpp (MarcoFalke)
Pull request description:
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
Fix it with a refactor and remove the suppression.
ACKs for top commit:
hebasto:
ACK fac62056b5, I have reviewed the code and it looks OK, I agree it can be merged.
prayank23:
Code Review ACK fac62056b5
Tree-SHA512: efc5b9887cb2e207033b264ebf425bae5ff013e909701c049aea5d79a21f10495826e962d171b3d412717cbf0a4723e5124133b5401b35a73915212e85e91020
aeb18b665c Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr)
b1a544be10 Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr)
2385b508d5 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr)
Pull request description:
1. Use a CSS selector to avoid changing the background colour of the tooltip.
2. Re-check validity of input when we first set the validator (probably a no-op in practice).
3. Check validity of input when it is set programmatically via `setText` (eg, via the address book). Probably no-op in practice UNTIL merging https://github.com/bitcoin/bitcoin/pull/15987 or any other PR that adds a warning for valid addresses.
Moved from https://github.com/bitcoin/bitcoin/pull/18133 (just concept ACKs)
ACKs for top commit:
Sjors:
tACK aeb18b665c
hebasto:
ACK aeb18b665c, tested on Linux Mint 20.3 (Qt 5.12.8).
Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f
99de8068cd validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() (Vasil Dimov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/24103 added annotations to
denote that the callers of `CChainState::ActivateBestChain()` and
`CChainState::InvalidateBlock()` must not own `m_chainstate_mutex` at
the time of the call.
Replace the added `LOCKS_EXCLUDED()` with a stronger
`EXCLUSIVE_LOCKS_REQUIRED()`, see
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative for the
difference between both.
ACKs for top commit:
hebasto:
ACK 99de8068cd.
jonatack:
ACK 99de8068cd. Tested with Debian clang version 13.0.1. Reproduced hebasto's results. Verified that `LoadExternalBlockFile()` needs the annotation added here.
Tree-SHA512: 59640d9ad472cdb5066ecde89cc0aff8632a351fc030f39bb43800d2c856fb1aed3576e4134212d32be161b18780f06dc5066ac71df7f7cd69e3f21f886e1542
b9c113af75 util: Avoid buggy std::filesystem:::create_directories() call (Hennadii Stepanov)
Pull request description:
Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04) [`std::filesystem:::create_directories()`](https://en.cppreference.com/w/cpp/filesystem/create_directory) call [fails](https://github.com/bitcoin/bitcoin/issues/24257#issue-1123753243) to handle symbol links properly.
No behavior change in comparison to the [pre-20744](c194293883) master branch.
Fixesbitcoin/bitcoin#24257.
ACKs for top commit:
ryanofsky:
Code review ACK b9c113af75. Nice simplification and fix
MarcoFalke:
review ACK b9c113af75🐬
Tree-SHA512: 79d940cfc1f68d9b0548fb2ab005e90850b54ac0fb3bb2940afd632d56288d92687579a3176bac3fd0ea3d2dae71e26444f8f7bdb87862414c12866ae5e857c4
fadcd03139 Fix unsigned integer overflow in LoadMempool (MarcoFalke)
Pull request description:
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
This removes one of the two violations.
This should be a refactor.
ACKs for top commit:
prayank23:
Code Review ACK fadcd03139
Tree-SHA512: 9fb2f3d49008a59cd45b7c17be0c88c04e61183197c11c8176865af5532c8d0c940db49a351dd0fc75e1d7fd8678c3b816d34cfca170dc6b9cf8f37fdf1c8cae
fad81548fa test: Avoid testing negative block heights (MarcoFalke)
Pull request description:
A negative chain height is only used to denote an empty chain, not the height of any block.
So stop testing that and remove a suppression.
ACKs for top commit:
brunoerg:
crACK fad81548fa
Tree-SHA512: 0f9e91617dfb6ceda99831e6cf4b4bf0d951054957c159b1a05a178ab6090798fae7368edefe12800da24585bcdf7299ec3534f4d3bbf5ce6a6eca74dd3bb766
fadc54b79b Fix unsigned integer overflow in tapscript validation weight calculation (MarcoFalke)
Pull request description:
Change the tapscript validation weight constants from uint64_t to int64_t, since the type of m_validation_weight_left is also int64_t. Otherwise this will cause sanitizer warnings.
This should be safe because signed integer overflow isn't expected to happen.
ACKs for top commit:
PastaPastaPasta:
utACK fadc54b79b
theStack:
Code-review ACK fadc54b79b
Tree-SHA512: 7a62d3a84733ab7827e3fa50d83f5493f2481b725c587e986eb2c128a769f023756f3ad964401526e386a847aa630a9f6c43a57d25ce5fd4af0b6bb5e0615528
d216bc8d76 Re-enable walletinit_verify_walletdir_no_trailing2 test disabled in #20744 (Ryan Ofsky)
80cd64e842 Re-enable util_datadir check disabled in #20744 (Ryan Ofsky)
Pull request description:
Reenable some broken tests as discussed https://github.com/bitcoin/bitcoin/pull/20744#discussion_r798651736 and https://github.com/bitcoin/bitcoin/pull/20744#discussion_r798678137
Fix windows test cases broken in #20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests.
It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it's not clear because I only tested on wine.
ACKs for top commit:
hebasto:
ACK d216bc8d76, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2099ddfa1a3ad70f7ac2ff413929414a1851d257b280da25c0f5cefb46fd1372b580a1f1ee5280681a1c16e6031f119185cadd4f7a6121298562cf001f711068
824e1ffa9f bench: Represents paths with fs::path instead of std::string (Ryan Ofsky)
Pull request description:
Suggested https://github.com/bitcoin/bitcoin/pull/20744#issuecomment-1022486215
ACKs for top commit:
fanquake:
untested ACK 824e1ffa9f
hebasto:
ACK 824e1ffa9f, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: 348fc189f30b5ad9a8e49e95e535d2c044462a9d534c3f34d887fbde0c05c41e88e02b4fc340709e6395a1188496a8333eb9e734310aa4c41755ec080e53c06e
This should also fix an init error if a -walletdir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This change passes canonical
paths to fs calls validating the -walletdir path to fix this.
This should also fix an assert error if a -datadir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This fix adds a
path::lexically_normal() call to the failing assert so it passes.
985d85e9a8 Follow Transifex docs to prepare XLIFF source (Hennadii Stepanov)
Pull request description:
This PR is a #21694 follow up.
From the Transifex [docs](https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file):
> A source file is different than a translation file. The translation file contains \<Target> references, whereas a source file does not.
This PR makes the `qt/locale/bitcoin_en.xlf` source file according to the docs.
ACKs for top commit:
laanwj:
ACK 985d85e9a8
Tree-SHA512: 537ef78f39a12f094b17fc902c5b59c7ae4d27e5fa35fbf6b33386843e4521236dac3275ba26ff2d1c435e53e8942284644d5de67b0b3543cec96cbcd4351607
f7a19ef774 qt,refactor: Use std::chrono in TrafficGraphWidget class (Shashwat)
Pull request description:
The PR is a follow-up to #517
- It addresses the change suggested in [this](https://github.com/bitcoin-core/gui/pull/517#pullrequestreview-850260826) comment.
- This PR changes the type of `msecsPerSample` from **int** to **std::chrono::minutes** and makes other relevant subsequent changes that were limited to the **trafficgraphwidget** file.
ACKs for top commit:
RandyMcMillan:
tACK f7a19ef774
hebasto:
ACK f7a19ef774
promag:
Code review ACK f7a19ef774.
Tree-SHA512: 5094ba894f3051fc99148cb8f408fc6f9d6571188673dcb7bf24366cdfb3eaf6d4e41083685d578ad2a9fbe31cc491a5f3fa9b7c9ab6eb90e4dc1356f89ae18a
04255073bb qt: Update source translations (laanwj)
cf79c56e65 init: Remove confusing '(possible integer overflow?)' from error message (laanwj)
d570a63894 qt: Update transifex resource blob to 23.0 (laanwj)
Pull request description:
- Update translations for 0.23 string freeze
- Update transifex resource blob to 23.0
This is necessary before a 23.0 resource can be created on Transifex.
ACKs for top commit:
hebasto:
ACK 04255073bb
Tree-SHA512: ff886e92721f070e3c135cfec229c41848a67c02355b88f2a5a507241b545f4209167d83b561420c2a82f49a5994170b01dcfb95bfc3fe6b9c832abcc6547b14
ef5014d256 style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b683491648 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac net: use Sock in CNode (Vasil Dimov)
c5dd72e146 fuzz: move FuzzedSock earlier in src/test/fuzz/util.h (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
This will help mocking / testing / fuzzing more code.
ACKs for top commit:
jonatack:
re-ACK ef5014d256 changes since last review are the removal of an unneeded dtor and the addition of a style commit
w0xlt:
reACK ef5014d
PastaPastaPasta:
utACK ef5014d256, I have reviewed the code, and believe it makes sense to merge
theStack:
Cod-review ACK ef5014d256
Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5
