fa2547fc52b90b4bbde250803df24d7f665383a7 fuzz: Avoid timeout in blockfilter fuzz target (MarcoFalke)
Pull request description:
Previously it would take 10 seconds to run this input, now it takes 10ms: [clusterfuzz-testcase-blockfilter-5022838196142080.log](https://github.com/bitcoin/bitcoin/files/7021883/clusterfuzz-testcase-blockfilter-5022838196142080.log)
The fix is moving the `MatchAny` out of the hot loop.
Also, to avoid unlimited runtime, cap the hot loop at 30k iterations.
ACKs for top commit:
GeneFerneau:
Approach ACK [fa2547f](fa2547fc52)
Tree-SHA512: a04e7388856930ec81222da8f05b665a923fe9482aeb4c55c9be4561aa7320a0703dbbf8d438ae92854e877a8e3b46777a29c0b652b8f34c29c2142cc5d63ccb
0d9fdd329e81cb171d687042290f4e6b1507d7f4 test, doc: refer to the correct variable names in p2p_invalid_tx.py (aitorjs)
Pull request description:
_tx_orphan_no_fee_ and _tx_orphan_invalid_ don't exist as transactions.
Have been replaced by _tx_orphan_2_no_fee_ and _tx_orphan_2_invalid_ respectively.
**Motivation**: Comments are more accurate and easy understandable under the tests context (I think).
ACKs for top commit:
kristapsk:
utACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4
theStack:
ACK 0d9fdd329e81cb171d687042290f4e6b1507d7f4 📃
Tree-SHA512: a4cafd931e51fe2a67085e10e9c61178c864c14982664d112b76327e040af08cd1de04eca4a8ae980fad57ba7078017ce02fc60e7658f38380e8172c2ae28b77
127b4608e9dbb8217c74c9332e82fcec8c326fa8 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6457f21596793a7149dc6dfea1dc871 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 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Zero-1729:
tACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
theStack:
Tested ACK 127b4608e9dbb8217c74c9332e82fcec8c326fa8
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
e1030a058c docs: add release notes for 6140 (pasta)
9ed292a6e1 feat: harden all sporks on mainnet to current values (pasta)
Pull request description:
## Issue being fixed or feature implemented
Harden all sporks on the mainnet; they are no longer necessary. Although retaining them might be beneficial in addressing bugs or issues, the greater priority is to protect mainnet by minimizing risks associated with potential centralization or even its perception. Sporks will continue to be valuable for testing on developer networks; however, on mainnet, the risks of maintaining them now outweigh the benefits of retaining them.
## What was done?
Adjust CSporkManager::GetSporkValue to always return 0 for sporks in general and 1 for SPORK_21_QUORUM_ALL_CONNECTED specifically.
## How Has This Been Tested?
Ran main net node with this patch. Sporks show as expected
## Breaking Changes
This is not a breaking change.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK e1030a058c
UdjinM6:
utACK e1030a058c (CI failure is unrelated)
Tree-SHA512: f20d0f614b7e9d6eb5606c545d0747a9d415f2905512dd6100a2f9fb00bb6de02c8d5aa74cb41aa39163fde0ab05fe472913acc227b1b4afce7e984f8897940d
1bf0bf492f merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh)
5c1eb67c42 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh)
c440304c85 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh)
e303a4ec45 merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh)
301163c65e merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh)
732e871a6b merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh)
b402fd57fa merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh)
a08f2f48bf merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh)
472caa048a merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh)
d69ca833df merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh)
6df927fc60 chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6078
* Dependent on https://github.com/dashpay/dash/pull/6074
* Dependent on https://github.com/dashpay/dash/pull/6083
* Dependent on https://github.com/dashpay/dash/pull/6119
* Dependency for https://github.com/dashpay/dash/pull/6138
* In [bitcoin#24050](https://github.com/bitcoin/bitcoin/pull/24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic.
* Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](37e026a038/src/spork.cpp (L44))) and upstream's usage of it to process translatable strings ([source](37e026a038/src/util/translation.h (L55-L62))).
Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore.
## Breaking Changes
None expected
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 1bf0bf492f (with one nit)
knst:
utACK 1bf0bf492f
PastaPastaPasta:
utACK 1bf0bf492f
Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
bebf391815 fix: build with -Wdocumentation - rename param pwallet to wallet (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
CI failures like that:
https://gitlab.com/dashpay/dash/-/jobs/7400661581
It happened due to 2 PRs merged one after each other without rebasing:
- enabling -Wdocumentation in bitcoin#21631 (#6113)
- renaming param while doxygen comment is forgotten in #6114
## What was done?
It contains changes from https://github.com/dashpay/dash/pull/6133
Thanks Vijay for spotting issue, this PR is DNM if 6133 will get merged faster.
## How Has This Been Tested?
See CI
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK bebf391815
PastaPastaPasta:
utACK bebf391815
Tree-SHA512: 34265f09098c04593a9d52709e5b8aed8460248762655945e5c86a65adb9aa49ab6f0bdb21cd907d353fe6b10e1ff2e07d05528166bf8e2140eb2c9ea1acbd33
We use Stretched column width, it's not compatible with gui#205
Though, this type of columns have better UI, but it is very slow performance,
see https://github.com/dashpay/dash/pull/6111#pullrequestreview-2178370404
-tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Date, QHeaderView::Interactive);
-tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Label, QHeaderView::Stretch);
+tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Date, QHeaderView::ResizeToContents);
+tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Label, QHeaderView::Interactive);
tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Message, QHeaderView::Stretch);
-tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Amount, QHeaderView::Fixed);
+tableView->horizontalHeader()->setSectionResizeMode(RecentRequestsTableModel::Amount, QHeaderView::ResizeToContents);
dc4551c22c46104d18e8e7a8bf133cbebe4bc89f remove incompatibility release note for darkmode on macos (Sylvain Goumy)
303cfc62277efccf242dc5a51368e349dc2782e3 allow darkmode on macos build (Sylvain Goumy)
78f75a2d60d4930e6e8d1b27030205179adf7f6d Allow icon colorization on mac os to better support dark mode (Uplab)
Pull request description:
Allow icons to be colorized on macOS to support native Dark mode color scheme.
Rendering on macOS Big Sur before PR:
Rendering on macOS Big Sur after PR:
Light mode stay visually unchanged.
<del>Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see https://github.com/bitcoin/bitcoin/pull/14593). </del>
<del>But once all glitches are fixed, we will be able to remove this temporary fix. </del>
Edit: this PR is know including the removal of `NSRequiresAquaSystemAppearance` on Info.plist file so that the color fix is apply to every build.
Linked issues: #68#136
ACKs for top commit:
hebasto:
re-ACK dc4551c22c46104d18e8e7a8bf133cbebe4bc89f
jarolrod:
ACK dc4551c22c46104d18e8e7a8bf133cbebe4bc89f
Tree-SHA512: 1c3a4dec796063e61fcaf80112afc2b15c8669a1cd30ebd537cea96647c20215f8f80289719f905820bb0c490c8c1f94bfae4bb32f9c6d1fdd4e8f199ebb559f
ef3e1d7272d294c65cc9904079571cf67dc6c463 qt: Improve URI/file handling message (Hennadii Stepanov)
Pull request description:
This PR:
- fixes missing spaces after full stops
- makes the translation context much bigger
The latter is the main motivation for this PR, as I became a translator 🐅
Screenshots:
- master (a9d1b40d53ec417eefbe767aa66701ef8e1801d5)
- this PR:
ACKs for top commit:
jarolrod:
ACK ef3e1d7272d294c65cc9904079571cf67dc6c463
Tree-SHA512: 8fbd1e3731b75866356fae201b3129126001600ca0197e83c05825e8c5bbbcf0132d6a6b808d7a5cbfbdde75ed1865ecbb651c30017570abd7c5803eff2b9306
d09ebc47233187ab8dd5a70df4d261353958978c Fix wrong(1024) divisor for 1000-based prefixes (wodry)
Pull request description:
v.0.21.0
I saw in the GUI peer window in the "received" column `1007 KB`, and after increasing to >=1024 I guess, it switched to `1 MB`. I would have expected the display unit to change from KB to MB already at value >=1000.
I looked into the code, and the values appear to be power-of-2 byte values, so the switching at >=1024 and not >=1000 seems correct.
But the unit display is not precisely correct, binary prefixes should be used for power-of-2 byte values.
To be correct, this PR changes ~~KB/MB/GB to KiB/MiB/GiB.~~ KB to kB and the divisor from 1024 to 1000.
ACKs for top commit:
hebasto:
ACK d09ebc47233187ab8dd5a70df4d261353958978c, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window".
jarolrod:
ACK d09ebc47233187ab8dd5a70df4d261353958978c
leonardojobim:
Tested ACK d09ebc4723 on Ubuntu 20.04 Qt 5.12.8
Tree-SHA512: 8f830b08cc3fd36dc8a18f1192959fe55d1644938044bf31d770f7c3bf8475fba6da5019a2d2024d5b2c81a8dab112f360c555367814a14f4d05c89d130f25b0
6242beeb067139c01dd27c63ebcd24df5808cb15 Hoist repeated translated strings to RPCConsole struct members (Jon Atack)
0f035c12fb0a5c5f98fc2b9907d475c08018df36 RPCConsole::updateDetailWidget: convert strings to translated strings (Jon Atack)
Pull request description:
- fixups from #206 review feedback (thanks!), see commit message for details
- hoists repeatedly used translatable strings to the `RPCConsole` class for reuse
ACKs for top commit:
hebasto:
re-ACK 6242beeb067139c01dd27c63ebcd24df5808cb15
Talkless:
tACK 6242beeb067139c01dd27c63ebcd24df5808cb15, tested on Debian Sid with Qt 5.15.2. I see "Ban for.." translated to my native language as before, "To/From/Yes/No" are not but that's expected, as `.ts` files are not updated.
jarolrod:
ACK 6242beeb067139c01dd27c63ebcd24df5808cb15
Tree-SHA512: 20a296511c5ac03a816766237fa2731b0360dedebf1bea02711eb21d7e4eae2a63a051fe48f4726052edc3e6318952f01fef920cd4b22a8196c39c23d8e5cc3a
It happened due to 2 PRs merged one after each other without rebasing:
- enabling -Wdocumentation in bitcoin#21631 (#6113)
- renaming param while doxygen comment is forgotten in #6114
4731f7045f docs: release notes for bitcoin#21141 - walletnotify %h %b (Konstantin Akimov)
044ddb4c80 Merge bitcoin/bitcoin#21777: test: Fix feature_notifications.py intermittent issue (MarcoFalke)
5336f42ea8 Merge bitcoin/bitcoin#19801: test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65) (MarcoFalke)
1cc6aa6c83 Merge bitcoin/bitcoin#21691: test: Check that no versionbits are re-used (MarcoFalke)
b55fdf8e68 Merge #21235: p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool (MarcoFalke)
ddc6fca7f3 Merge #21343: doc: revamp macOS build doc (fanquake)
709652bff7 Merge #21141: wallet: Add new format string placeholders for walletnotify (Wladimir J. van der Laan)
a3bee9c8ec Merge #21424: Net processing: Tidy up CNodeState ctor (MarcoFalke)
aab2a665c3 Merge #20556: rpc: Properly document return values (submitblock, gettxout, getblocktemplate, scantxoutset) (fanquake)
c26722fc0e Merge #21331: rpc: replace wallet raw pointers with references (#18592 rebased) (MarcoFalke)
Pull request description:
## What was done?
Backports from bitcoin v22:
- bitcoin/bitcoin#21331
- bitcoin/bitcoin#20556
- bitcoin/bitcoin#21424
- bitcoin/bitcoin#21141
- bitcoin/bitcoin#21343
- bitcoin/bitcoin#21235
- bitcoin/bitcoin#21691
- bitcoin/bitcoin#19801
- bitcoin/bitcoin#21777
## How Has This Been Tested?
Run unit and functional tests
## Breaking Changes
N/A
## Checklist:
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 4731f7045f
PastaPastaPasta:
utACK 4731f7045f
Tree-SHA512: dcee684563e4e07e838e96df0bf5e49d59e8c85aea6beca3239782e7d90a24f13d828b1201073585b888b0b154b4ba8bb90f88a36e3f923ac03b46f595d75500
121c032e41 fix: avoid calling functions that change wallet state inside of `assert(...)` (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
Functions that change the state should not be called inside `assert`s
kudos to @kwvg for noticing https://github.com/dashpay/dash/pull/6116#discussion_r1681163803
## What was done?
Move them out
## How Has This Been Tested?
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
kwvg:
LGTM, utACK 121c032e41
PastaPastaPasta:
utACK 121c032e41
Tree-SHA512: 42b6f55a62b05e3e632febf92f9d5ac63f32a7754fdd9dffa816d71cb0f72a32ac8315fba5ed96d8bc652728e768f2eb399e5ebb4aa39afe1546fa16e1046347
5943c93bdd feat: introduce `coinjoinsalt` RPC to allow manipulating per-wallet salt (Kittywhiskers Van Gogh)
4fa3d396f4 wallet: refactor `InitCoinJoinSalt()` to allow for setting a custom salt (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
CoinJoin utilizes a salt to randomize the number of rounds used in a mixing session (introduced in [dash#3661](https://github.com/dashpay/dash/pull/3661)). This was done to frustrate attempts at deobfuscating CoinJoin mixing transactions but also has the effect of deciding the mixing threshold at which a denomination is considered "sufficiently mixed", which in turn is tied this salt, that is in turn, tied to the wallet.
With wallets that utilized keypool generation, this was perfectly acceptable as each key was unrelated to the other and any meaningful attempts to backup/restore a wallet would entail backing up the entire wallet database, which includes the salt. Meaning, on restore, the wallet would show CoinJoin balances as reported earlier.
With the default activation of HD wallets in legacy wallets (and the introduction of descriptor wallets that construct HD wallets by default), addresses are deterministically generated and backups no longer _require_ a backup of the wallet database wholesale.
Users who export their mnemonic and import them into a new wallet will find themselves with a different reported CoinJoin balance (see below).
https://github.com/dashpay/dash/assets/63189531/dccf1b17-55af-423d-8c36-adea6163e060
## Demo
**Based on [`c00a3665`](c00a366578)**
https://github.com/dashpay/dash/assets/63189531/c60c10e3-e414-46af-a64e-60605a4e6d07
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
ACK 5943c93bdd
Tree-SHA512: 8e5bdd18ecce4c14683fbeb1812b0b683a5a5b38405653655c3a14b1ab32a22b244b198087876cd53ca12447c2027432afe4259ef3043be7fddf640911d998f0
e65984e173 refactor: remove unused includes from unit tests (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Noticed there were some not used includes in Unit tests and removed them.
It's not exhaustive list of unused headers, just something that was easy to spot.
## How Has This Been Tested?
Just build and see that there's no build error.
This PR also slightly reduced compilation time of project, it's measurable big changes to6-8 seconds of one CPU core faster. Tested by compiling only files that changed in single-thread:
```
$ rm src/test/test_dash-blockfilter_index_tests.o src/test/test_dash-block_reward_reallocation_tests.o src/test/test_dash-denialofservice_tests.o src/test/test_dash-dynamic_activation_thresholds_tests.o src/test/test_dash-validation_block_tests.o src/test/test_dash-validation_chainstate_tests.o src/test/test_dash-validation_flush_tests.o src/test/util/libtest_util_a-mining.o src/test/util/libtest_util_a-setup_common.o
$ cd src/test ; time make -j1
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK e65984e173
PastaPastaPasta:
utACK e65984e173
Tree-SHA512: 445c1515aee09e946eb73f12ca9c8064d26cd6148d2286fa079bf54806ea77af9eda890a36c8f34450791e868fbe933183bc7e507dd6e667304ccd92790bb009
252ffee815 fix: adjust doxygen for dash codebase for -Wdocumentation (Konstantin Akimov)
4f260cd4b1 fix: ignore warnings for dashbls/ (Konstantin Akimov)
0afffcccc4 Merge #21613: build: enable -Wdocumentation (fanquake)
Pull request description:
## What was done?
Backport bitcoin#21613 to enable -Wdocumentation and related fixes for dash code
## How Has This Been Tested?
Build with clang compiler and see no failures anymore with `--enable-werror`
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
UdjinM6:
utACK 252ffee815
PastaPastaPasta:
utACK 252ffee815
Tree-SHA512: 1670076665e26228567042ed33ce149780c3992aaa78cdeafeefdc5609b1d1590034a3a2ddc056b69120b2b4e8b0dffaff729b8178043aebe19b1c5880a10d0d
Bitcoin uses underscore in util/translation.h for translatable strings
but underscores are also a placeholder used in multiple languages, incl.
C++. The next commit will be introducing backports, one of them will be
placing util/translation.h into validation.h, which is a header used by
Dash-specific code.
This causes a conflict between the normal usage of underscore as a
placeholder and Bitcoin's usage of it as a function name, which is
reported by the Dash-specific linter. We need to exclude shadowing
warnings from the linter to account for this.
b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)
Pull request description:
The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:
f8462a6d27/test/functional/feature_cltv.py (L26-L35)
This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
* _the stack is empty_
➡️ prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
* _the top item on the stack is less than 0_
➡️ prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
* _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
* _the top stack item is greater than the transaction's nLockTime field_
➡️ prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
* _the nSequence field of the txin is 0xffffffff_
➡️ prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500
The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").
ACKs for top commit:
MarcoFalke:
review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣
Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
fa8eaee6a8531db970cc84436bf2ae8150a58642 test: Run versionbits_sanity for all chains (MarcoFalke)
faec1e9ee1f12612831ad5b0f0a767d87bd2d024 test: Address outstanding versionbits_test feedback (MarcoFalke)
fad4167871c3c9fde462e64e3ef3be937e585084 test: Check that no versionbits are re-used (MarcoFalke)
Pull request description:
ACKs for top commit:
jnewbery:
Code review ACK fa8eaee6a8531db970cc84436bf2ae8150a58642
ajtowns:
ACK fa8eaee6a8531db970cc84436bf2ae8150a58642 code review only
Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
fa8177324392c923c6ce39056cfd870af55ab673 style-only: Remove whitespace (MarcoFalke)
fae77b9e6dc9e59b355d56df49c4d9685b6f40a4 net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman)
fae7c0429f96e08bcac944f6fa30264636dfda8c log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects (MarcoFalke)
Pull request description:
* Clarify that "ignoring" really means "disconnect" in the log
* Revive a refactor I took from #13670
ACKs for top commit:
jnewbery:
utACK fa8177324392c923c6ce39056cfd870af55ab673
sipa:
utACK fa8177324392c923c6ce39056cfd870af55ab673
Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
c180c911b88b2bd2baf2c9c2b24e276787ffb69b doc: revamp macOS build doc (Jarol Rodriguez)
Pull request description:
This PR makes the macOS build-docs more informative and adds in the following information:
- Proper descriptions and delineation of required/optional dependencies
- walk-through of optional dependencies
- configuration walk-through
- various other tidbits of information
This is a part of the efforts done in https://github.com/bitcoin/bitcoin/pull/20601 and https://github.com/bitcoin/bitcoin/pull/20610 to update the docs and introduce some consistency between them.
This update does not add instructions for arm-based M1 Macbooks as I do not have one to test with. It would be nice to have someone follow up with an update containing instructions for arm-based Macs.
**Before/Master:** [render](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md)
**After/PR:** [render](c180c911b8/doc/build-osx.md)
ACKs for top commit:
fanquake:
ACK c180c911b88b2bd2baf2c9c2b24e276787ffb69b - I still think these are getting too verbose and we're duplicating information all over the place; dependencies, configure options, combinations of options etc. However if people are happy to maintain them, I guess it's fine for now, and this revamping has already happened for some of the other build READMEs.
Tree-SHA512: 1440046c723fe80d4158e4a429e3aa8bd93570acb84ad202d5d24c749ab9a89a3aca8b61b49e75e042a4bf4317acd632d3906e1b5808a9052e74209256528b45
06e1fb0b170a69996a7ce1ef5203785a7bc6b278 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet)
Pull request description:
This patch includes two new format placeholders for walletnotify:
%b - the hash of the block containting the transaction (zeroed if a mempool transaction)
%h - the height of the block containing the transaction (zero if a mempool transaction)
I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes.
**Motivation**
The walletnotify option is used to be notified of new transactions relevant to the wallet of the node.
A common usage pattern is to perform afterwards additional RPC calls to determine:
1. If this is a mempool transaction or not (i.e. are there any confirmations?)
2. What block was it included in?
3. Did this transaction was seen before and is now seen again because of a fork?
All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.
Please let me know of any questions and suggestions.
ACKs for top commit:
laanwj:
ACK 06e1fb0b170a69996a7ce1ef5203785a7bc6b278
Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
6927933782acb9b158787e6f35debb916793f6b1 [net processing] Add ChainSyncTimeoutState default initializers (John Newbery)
55966e0cc03f0e380d21a9434b048d4d515b6729 [net processing] Remove CNodeState ctor body (John Newbery)
Pull request description:
This addresses the two outstanding review comments from #21370.
ACKs for top commit:
practicalswift:
cr ACK 6927933782acb9b158787e6f35debb916793f6b1: patch looks correct
hebasto:
ACK 6927933782acb9b158787e6f35debb916793f6b1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: b3ef5c8a096e447887df255406b3a760f01c73e2b942374595416b4b4031fc69b89cd93168c45040489d581f340b2a62d3fbabd207d4307f587c00a7a7daacd1
fa7ff0790ec21d187f1a32310918b0c8d66e5266 rpc: Properly document submitblock return value (MarcoFalke)
fae542c28b269d4a8a39f48ef829734b1241dd4f rpc: Properly document getblocktemplate return value (MarcoFalke)
fabaccf031cfac718bf05b140f1901d93ee8be67 rpc: Properly document scantxoutset return value (MarcoFalke)
faa2059547389e342991ab0d9382f8694f74fce1 rpc: Properly document gettxout return value (MarcoFalke)
Pull request description:
Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476
ACKs for top commit:
fjahr:
utACK fa7ff0790ec21d187f1a32310918b0c8d66e5266
amitiuttarwar:
tACK fa7ff0790e
Tree-SHA512: 933cb8f003163d93dbedb302d4c162514c2698ec6d58dbb9a053da8b8b9a4459b0701a3d9e830ecdabd7f278a46b7a07a3af49ec60703a80fcd75390877294ea
7c90c67b7e6f598f9ffdc136ded2b533b78ed044 rpc: refactor rpc wallet functions to take references instead of pointers (fanquake)
48669340080feaff86b8fc0403ef22c820477697 rpc: remove calls to CWallet.get() (fanquake)
Pull request description:
This is a rebased #18592.
> This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here https://github.com/bitcoin/bitcoin/issues/18590
> It seems that this PR is indirectly related to this issue: https://github.com/bitcoin/bitcoin/pull/13063#discussion_r186740049
> Notice: I have deliberately **not** changed the class `WalletRescanReserver ` whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral".
> Fixes https://github.com/bitcoin/bitcoin/issues/18590
ACKs for top commit:
MarcoFalke:
ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044 🐧
ryanofsky:
Code review ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044. Changes easy to review with `--word-diff-regex=. -U0`
Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
bbc99571f3 fix: sidestep c++17 std::unary_function removal by compiling boost with c++11 (Kittywhiskers Van Gogh)
3c622a3916 revert: partial dash#5610 (make depends compilable with Xcode 15 on macos) (Kittywhiskers Van Gogh)
f15e1db477 fix: make `std::unary_function` suppression flag no longer contingent on `--enable-suppress-external-warnings` (Kittywhiskers Van Gogh)
5db84e2a5b revert: partial dash#3003 (Fix 2 common Travis failures which happen when Travis has network issues) (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Despite builders for BSD-based platforms being backported as early as [dash#5362](https://github.com/dashpay/dash/pull/5362), they didn't work on account of using a `curl` flag, `--retry`, that was lopped off other builders in [dash#3003](https://github.com/dashpay/dash/pull/3003) as a way to mitigate aberrant Travis CI-specific behaviour.
As the variable `$(DOWNLOAD_RETRIES)` was removed as part of that mitigation, the introduction of builders that rely on this variable caused failures as the flag was accompanied by nothing. As our CI host isn't based on FreeBSD, this went unnoticed. When deciding between extending the existing patch and reverting it, reverting it proved to be more attractive on account of us no longer using Travis CI and the revert bringing us closer to upstream.
Additionally, the `std::unary_function` patch that was introduced in [dash#5610](https://github.com/dashpay/dash/pull/5610) proves to be necessary on BSD-based platforms as well (had to be extended for and tested on GhostBSD, based on FreeBSD 13.2-STABLE, Clang 16). Instead of conditionally patching based on platform and compiler, a more reliable patch would be to downgrade the C++ version used to build Boost at the last version to have `std::unary_function`, which would be C++11, albeit deprecated ([source](https://en.cppreference.com/w/cpp/utility/functional/unary_function)).
Though it's likely this route wasn't taken originally due to compiler errors that happened despite downgrading to C++11. These errors were due to the compiler objecting to `std::unary_function` usage in Boost headers, despite the backport of [bitcoin#25436](https://github.com/bitcoin/bitcoin/pull/25436), which should've solved this problem. The reason the errors were still persisting is because the necessary flag, `-DBOOST_NO_CXX98_FUNCTION_BASE`, was only applied if `--enable-suppress-external-warnings` was set. CI didn't catch this, as the flag is always set, to keep log lengths manageable. This has been rectified.
All changes combined, one should be able to build non-Qt Dash binaries using `depends` though building the Qt client from `depends` unfortunately remains a problem, even upstream ([source](https://github.com/bitcoin/bitcoin/pull/23955#issuecomment-1039300427), [source](https://github.com/bitcoin/bitcoin/pull/23948#issue-1092284497)).
## Demo
**Based on bbc99571f36ecfd817dc33b883c5e6f120240270**
Built using:
* **`depends` flags:** `NO_QT=1 ALLOW_HOST_PACKAGES=1 HOST=x86_64-unknown-freebsd13.2`
* **`configure` flags :** `--prefix=$(pwd)/depends/x86_64-unknown-freebsd13.2 --enable-debug --enable-suppress-external-warnings --with-gui`
* Qt installed using `pkg` (`qt5`)
## Checklist:
- [x] I have performed a self-review of my own code **(note: N/A)**
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
light ACK bbc99571f3
PastaPastaPasta:
utACK bbc99571f3
knst:
ACK bbc99571f3
Tree-SHA512: b29d6775f42965d2f09307aff0192012aa192e39e06a83b800613831dc00fc7173201d496117fbe86850f5208a0b7688a376f2ee618881c062c28d694085efc9
878bce0f45 docs: update SECURITY.md supported versions (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
Updates the supported versions list in `SECURITY.md`
## Checklist:
- [x] I have performed a self-review of my own code **(note: N/A)**
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 878bce0f45
UdjinM6:
utACK 878bce0f45
knst:
utACK 878bce0f45
Tree-SHA512: d941a3ca0792b2f08f68cab562a35d869d8e93f627918a25a9753955b6103d1515899b0ca50ff936c966b9f9fd603e12d27b03267361c8f1030a31f9fffdf2ae
bfc083e9b7 Merge #21574: Drop JSONRPCRequest constructors after #21366 (MarcoFalke)
c0cdb0488b Merge #21572: Fix wrong wallet RPC context set after #21366 (MarcoFalke)
2f7814acdd Merge #21035: Remove pointer cast in CRPCTable::dumpArgMap (MarcoFalke)
d3b1ef374c refactor: simplify implementation of RPC composite commands (Konstantin Akimov)
3270becc9b chore: add TODO to make client parsing for composite commands (Konstantin Akimov)
d55759fa79 Merge #20012: rpc: Remove duplicate name and argNames from CRPCCommand (Wladimir J. van der Laan)
1d87ce4e86 Merge #18531: rpc: remove deprecated CRPCCommand constructor (MarcoFalke)
a7e538d7ae fix: missing changes from bitcoin#19250 (Konstantin Akimov)
68c5da41dc feat: fix help message - all subcommands support it now! (Konstantin Akimov)
d3e181f516 fix: add missing client's argument parsing for RPC commands (Konstantin Akimov)
37bd4009c1 refactor: use monostate instead std::optional in CoreContext (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Backports from bitcoin v22 rpc command related
## What was done?
See commits for backports.
Also:
- refactored and significantly simplified implementation of composite commands
- added missing changes from bitcoin#19250
- fix help message for rpc `help` - all subcommands support it now
- add missing client's argument parsing for RPC commands
- CoreContext uses std::monostate instead nullopt which is best-practice for std::variant
## How Has This Been Tested?
Run unit/functional tests.
Checked autocomplete for various commands
Checked help for various commands
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK bfc083e9b7
UdjinM6:
utACK bfc083e9b7
Tree-SHA512: b7b586eac2f848a6808c677252a5965577bc783778fd70d3025b8d510113de2b177d423d72ea5f61ddd8905673bf3458e55810ada371ee235fbaa19de8d2d36f
69c37f4ec2 rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD` (Kittywhiskers Van Gogh)
619b640a77 wallet: unify HD chain generation in CWallet (Kittywhiskers Van Gogh)
163d31861c wallet: unify HD chain generation in LegacyScriptPubKeyMan (Kittywhiskers Van Gogh)
Pull request description:
## Motivation
When filming demo footage for https://github.com/dashpay/dash/pull/6093, I realized that if I tried to create an encrypted blank legacy wallet and run `upgradetohd [mnemonic]`, the client would crash.
```
dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
Posix Signal: Aborted
No debug information available for stacktrace. You should add debug information and then run:
dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===
```
The expected set of operations when performing privileged operations is to first use `walletpassphrase [passphrase] [time]` to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., `upgradetohd` imports **always** need the passphrase, if encrypted).
You might have noticed that I used `upgradetohd [mnemonic]` instead of the correct syntax, `upgradetohd [mnemonic] "" [passphrase]` that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.
```
Error: Please enter the wallet passphrase with walletpassphrase first.
```
Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so `EnsureWalletIsUnlocked()` is happy, so now the following happens.
```
upgradetohd()
| Insufficient validation has allowed us to supply a blank passphrase
| for an encrypted wallet
|- CWallet::UpgradeToHD()
|- CWallet::GenerateNewHDChainEncrypted()
| We get our hands on vMasterKey by generating the key from our passphrase
| and using it to unlock vCryptedMasterKey.
|
| There's one small problem, we don't know if the output of CCrypter::Decrypt
| isn't just gibberish. Since we don't have a passphrase, whatever came from
| CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the
| vMasterKey we just got is gibberish
|- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
|- res = LegacyScriptPubKeyMan::EncryptHDChain()
| |- EncryptSecret()
| |- CCrypter::SetKey()
| This is where everything unravels, the gibberish key's size doesn't
| match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
|- assert(res)
We assume are inputs are safe so there's no real reason we should crash.
Except our inputs aren't safe, so we crash. Welp! :c
```
This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (19512988c6), trying to do the same thing would return you a vague error
```
Failed to generate encrypted HD wallet (code -4)
```
In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to `upgradetohd`, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.
```
upgradetohd()
| We've been supplied a passphrase, so we can try and validate it by
| trying to unlock the wallet with it. If it fails, we know we got the
| wrong passphrase.
|- CWallet::Unlock()
| | Before we bother unlocking the wallet, we should check if we're
| | already unlocked, if we are, we can just say "unlock successful".
| |- CWallet::IsLocked()
| | Wallet is indeed unlocked.
| |- return true;
| The validation method we just tried to use has a bail-out mechanism
| that we don't account for, the "unlock" succeded so I guess we have the
| right passphrase.
[...] (continue call chain as mentioned earlier)
|- assert(res)
Oh...
```
This pull request aims to resolve crashes caused by the above two edge cases.
## Additional Information
As this PR was required me to add additional guardrails on `GenerateNewCryptedHDChain()` and `GenerateNewHDChainEncrypted()`, it was taken as an opportunity to resolve a TODO ([source](9456d0761d/src/wallet/wallet.cpp (L5028-L5038))). The following mitigations have been implemented.
* Validating `vMasterKey` size (any key not of `WALLET_CRYPTO_KEY_SIZE` size cannot be used for encryption and so, cannot be a valid key)
* Validating `secureWalletPassphrase`'s presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
* Using `Unlock()` to validate the correctness of `vMasterKey`. (the two other instances of iterating through `mapMasterKeys` use `Unlock()`, see [here](1394c41c8d/src/wallet/wallet.cpp (L5498-L5500)) and [here](1394c41c8d/src/wallet/wallet.cpp (L429-L431)))
* `Lock()`'ing the wallet before `Unlock()`'ing the wallet to avoid the `IsLocked()` bail-out condition and then restoring to the previous lock state afterwards.
* Add an `IsCrypted()` check to see if `upgradetohd`'s `walletpassphrase` is allowed to be empty.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK 69c37f4ec2
UdjinM6:
utACK 69c37f4ec2
PastaPastaPasta:
utACK 69c37f4ec2
Tree-SHA512: 4bda1f7155511447d6672bbaa22b909f5e2fc7efd1fd8ae1c61e0cdbbf3f6c28f6e8c1a8fe2a270fdedff7279322c93bf0f8e01890aff556fb17288ef6907b3e
