586190f0b4 rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong)
bc3bd36902 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong)
f99913969f rpc: Remove unnecessary casting of block height (Carl Dong)
6a3d192020 rpc: Tidy up local references (see commit message) (Carl Dong)
038854f31e rest/rpc: Remove now-unused old Ensure functions (Carl Dong)
6fb65b49f4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong)
1570c7ee98 rpc: Add renamed EnsureAny*() functions (Carl Dong)
306b1cd3ee rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong)
d7824acdb9 rest: Use existing NodeContext (Carl Dong)
3f08934799 rest: Pass in NodeContext to rest_block (Carl Dong)
7be0671b95 rpc/rawtx: Use existing NodeContext (Carl Dong)
60dc05afc6 rpc/mining: Use existing NodeContext (Carl Dong)
d485e815e2 rpc/blockchain: Use existing NodeContext (Carl Dong)
d0abf0bf42 rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong)
cced0f46c9 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules
- [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
ryanofsky:
Code review ACK 586190f0b4. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates!
jnewbery:
utACK 586190f0b4
MarcoFalke:
review ACK 586190f0b4🍯
Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
7f3a5980c1 qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)
Pull request description:
Windows and macOS do [not support](https://doc.qt.io/qt-5/qclipboard.html#notes-for-windows-and-macos-users) the global mouse selection.
Fixes#258.
ACKs for top commit:
promag:
Code review ACK 7f3a5980c1.
jarolrod:
ACK 7f3a5980c1
Tree-SHA512: be2beeef7d25af6f4d4a4548325d8d29f08e4342f499666bc4a670ed468a63195d514077c2cd0dba197e12bd43316fd3e2813cdc0954364b6aa4ae6b90c118bf
63631beef6 test: Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic` (practicalswift)
Pull request description:
Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic`.
Fixes#21682.
Rationale from https://github.com/bitcoin/bitcoin/issues/21682#issuecomment-819897122:
> I've looked at that test before and I don't think that specific `BOOST_CHECK` makes much sense TBH :)
>
> 1.) I don't understand why we test if `ToString()` output includes `%zone_index`: it clearly doesn't on some platforms, so we cannot rely on it anyways. Then why test it?
>
> 2.) And perhaps more fundamentally: why would we even _want_ to have `%zone_index` in our textual `ToString()` output? I think the expectation is to get say `fe80::1ff:fe23:4567:890a` (without zone index) and not say `fe80::1ff:fe23:4567:890a%eth2 ` or `fe80::1ff:fe23:4567:890a%3 `when doing `ipv6_addr.ToString()` :)
ACKs for top commit:
MarcoFalke:
review ACK 63631beef6
Tree-SHA512: 06863d1edfb9ad1ca9bcae09cf3f0f47b58bb29d222b70799c3dc059b96452889026e4b99b132782846d9896e3e798d17c7f9406e0e6a0bec1bffc6edb54e9df
549c82ad3a fuzz: use ConsumeBool() instead of !ConsumeBool() (Vasil Dimov)
29ae1c13a5 fuzz: split FuzzedSock interface and implementation (Vasil Dimov)
9668e43d8e fuzz: make FuzzedSock::Wait() sometimes simulate an occurred event (Vasil Dimov)
0c90ff1429 fuzz: set errno from FuzzedSock::Wait() if it simulates a failure (Vasil Dimov)
5198a02de4 style: remove extra white space (Vasil Dimov)
Pull request description:
* split FuzzedSock interface and implementation
* make FuzzedSock::Wait() sometimes simulate an occurred event
* set errno from FuzzedSock::Wait() if it simulates a failure
(this is a followup from https://github.com/bitcoin/bitcoin/pull/21617)
ACKs for top commit:
practicalswift:
cr ACK 549c82ad3a: patch looks correct and touches only `src/test/fuzz/`
MarcoFalke:
re-ACK 549c82ad3a only change is rebase 🎬
Tree-SHA512: 8ba965a8319074ad2ef840219c35c77e37cc79f00fb3926f20ccbf5f58e9616f5a3ac96434ad33996b47d292fa760d5d00a529001ac0d1d254262e5df93f616f
fa40d6a1c4 test: Reset mocktime in the common setup (MarcoFalke)
fa78590a8f test: Use mocktime to avoid intermittent failure (MarcoFalke)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/21602#discussion_r611176103
ACKs for top commit:
jonatack:
Code review ACK fa40d6a1c4
jarolrod:
ACK fa40d6a1c4
Tree-SHA512: 4967e006f3d2c4eb92f03c9086a6abe3190ad54755d251c30d20422c574bb1a154c06f3d5bcb0d4deaa3c4abfd3864d743b71d84897edd358e829bb42233ad12
Move the `FuzzedSock`'s implementation from `src/test/fuzz/util.h` to
`src/test/fuzz/util.cpp`.
A separate interface and implementation make the code more readable for
consumers who don't need to (better not) know the implementation
details.
6262182b3f Avoid use of low file descriptor ids (which may be in use) in FuzzedSock and StaticContentsSock (practicalswift)
Pull request description:
Avoid use of low file descriptor ids (which may be in use) in `FuzzedSock`.
Context: https://github.com/bitcoin/bitcoin/pull/21630/files#r610694541
ACKs for top commit:
vasild:
ACK 6262182b3f
Tree-SHA512: e622acb4d01446c3db01adbbbb779038be7247e13f3f4e72c614bc2880c3efd710fd3b189f87abb00f236fa5ddf91f4c215f420ca4eb08a97aaba31593254c3d
ffe33dfbd4 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2 versionbits: simplify state transitions (Anthony Towns)
55ac5f568a versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48 fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c tests: test versionbits delayed activation (Anthony Towns)
73d4a70639 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa tests: clean up versionbits test (Anthony Towns)
5932744450 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a47 tests: pull ComputeBlockVersion test into its own function (Anthony Towns)
Pull request description:
BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html
Edge cases are tested by fuzzing added in #21380.
ACKs for top commit:
instagibbs:
tACK ffe33dfbd4
jnewbery:
utACK ffe33dfbd4
MarcoFalke:
review ACK ffe33dfbd4💈
achow101:
re-ACK ffe33dfbd4
gmaxwell:
ACK ffe33dfbd4
benthecarman:
ACK ffe33dfbd4
Sjors:
ACK ffe33dfbd4
jonatack:
Initial approach ACK ffe33dfbd4 after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
ariard:
Code Review ACK ffe33df
Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
In all rest/rpc-related modules, if there are multiple calls to
ActiveChain{,State}(), and the calls fall under the same ::cs_main lock,
we can simply take a local reference and use/reuse it instead of calling
ActiveChain{,State}() again and again.
Organize local variables/references such that:
1. There is always a `ChainstateManager` reference before any `LOCK(cs_main)`.
2. NodeContext references are used with Ensure*() functions introduced in
previous commit where appropriate to avoid duplicate assertions.
The scripted-diff in the previous commit should have removed all calls
to functions like: Ensure(?!Any)\(const std::any& (context|ctx)\), so we
can remove them now.
- The original Ensure*(const std::any& context) functions are kept and
the parameter renamed to ctx so that the scripted-diff in the
subsequent commit will work as expected
- The renaming avoids overloading mistakes arising out of the untyped
std::any argument.
fa73ce6e65 Fix assumeutxo crash due to truncated file (MarcoFalke)
Pull request description:
ACKs for top commit:
jamesob:
ACK fa73ce6e65
ryanofsky:
Code review ACK fa73ce6e65. Easy fix. It seems like this could have been caught in review, though.
Tree-SHA512: 3a98687c386e3995114ddf0ad7194fadd9520989290681ef703b578e3ca21aee51eadfb83aa38a489bac13d12709ea137b9b184b08e5bfa2919cca177aab90be
b8e5d0d3fe qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot (Hennadii Stepanov)
1ac2bc7ac0 qt: Handle exceptions in TransactionView::bumpFee slot (Hennadii Stepanov)
bc00e13bc8 qt: Handle exceptions in WalletModel::pollBalanceChanged slot (Hennadii Stepanov)
eb6156ba1b qt: Handle exceptions in BitcoinGUI::addWallet slot (Hennadii Stepanov)
f7e260a471 qt: Add GUIUtil::ExceptionSafeConnect function (Hennadii Stepanov)
64a8755af3 qt: Add BitcoinApplication::handleNonFatalException function (Hennadii Stepanov)
af7e365b15 qt: Make PACKAGE_BUGREPORT link clickable (Hennadii Stepanov)
Pull request description:
This PR is an alternative to https://github.com/bitcoin/bitcoin/pull/18897, and is based on Russ' [idea](https://github.com/bitcoin/bitcoin/pull/18897#pullrequestreview-418703664):
> IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods ...
Related issues
- #91
- https://github.com/bitcoin/bitcoin/issues/18643
Qt docs: https://doc.qt.io/qt-5.12/exceptionsafety.html#exceptions-in-client-code
With this PR the GUI handles the wallet-related exception, and:
- display it to a user:
- prints a message to `stderr`:
```
************************
EXCEPTION: 18NonFatalCheckError
wallet/wallet.cpp:2677 (IsCurrentForAntiFeeSniping)
Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues
bitcoin in QPushButton->SendCoinsDialog
```
- writes a message to the `debug.log`
- and, if the exception is a non-fatal error, leaves the main window running.
ACKs for top commit:
laanwj:
Code review ACK b8e5d0d3fe
ryanofsky:
Code review ACK b8e5d0d3fe. This is great! I think more improvements are possible but implementation is very clean and I love how targeted each commit is. Changes since last review: adding more explanatory text, making links clickable, reorganizing.
Tree-SHA512: a9f2a2ee8e64b993b0dbc454edcbc39c68c8852abb5dc1feb58f601c0e0e8014dca81c72733aa3fb07b619c6f49b823ed20c7d79cc92088a3abe040ed2149727
41f891da50 tests: Skip SQLite fsyncs while testing (Andrew Chow)
Pull request description:
Since we want tests to run quickly, and since tests do a lot more db operations than expected we expect to see in actual usage, we disable sqlite's syncing behavior to make db operations run much faster. This syncing behavior is necessary for normal operation as it helps guarantee that data won't become lost or corrupted, but in tests, we don't care about that.
Fixes#21628
ACKs for top commit:
vasild:
ACK 41f891da50
Tree-SHA512: f36f969a182c622691ae5113573a3250e8d367437e83a1a9d3d2b55dd3a9cdf3c6474169a7bd271007bb9ce47f585aa7a6aeae6eebbaeb02d79409b02f47fd8b
fadcd3f78e doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad2 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3 move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a1 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f refactor: Move load block thread into ChainstateManager (MarcoFalke)
Pull request description:
This picks up the closed pull request #21030 and is the first step toward fixing #21220.
The basic idea is to move all disk access into a separate module with benefits:
* Breaking down the massive files init.cpp and validation.cpp into logical units
* Creating a standalone-module to reduce the mental complexity
* Pave the way to fix validation related circular dependencies
* Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)
ACKs for top commit:
promag:
Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
jamesob:
ACK fadcd3f78e ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
ryanofsky:
Code review ACK fadcd3f78e. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.
Tree-SHA512: 917996592b6d8f9998289d8cb2b1b78b23d1fdb3b07216c9caec1380df33baa09dc2c1e706da669d440b497e79c9c62a01ca20dc202df5ad974a75f3ef7a143b
003929c0d5 refactor: add [[noreturn]] attribute where applicable (fanquake)
Pull request description:
Similar to #10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.
ACKs for top commit:
vasild:
ACK 003929c0d5
Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
Rather than 3 different messages that are confusing / leak
implementation details, use a single message, that is similar to other
wallet related messages. i.e:
"Compiled without sqlite support (required for descriptor wallets)".
88d4d5ff2f rpc: add help for enumeratesigners and walletdisplayaddress (Sjors Provoost)
b0db187e5b ci: use --enable-external-signer instead of --with-boost-process (Sjors Provoost)
b54b2e7b1a Move external signer out of wallet module (Sjors Provoost)
Pull request description:
In addition, this PR enables external signer testing on CI.
This PR moves the ExternalSigner class and RPC methods out of the wallet module.
The `enumeratesigners` RPC can be used without a wallet since #21417. With additional modifications external signers could be used without a wallet in general, e.g. via `signrawtransaction`.
The `signerdisplayaddress` RPC is ranamed to `walletdisplayaddress` because it requires wallet context. A future `displayaddress` RPC call without wallet context could take a descriptor argument.
This commit fixes a `rpc_help.py` failure when configured with `--disable-wallet`.
ACKs for top commit:
ryanofsky:
Code review ACK 88d4d5ff2f
fanquake:
ACK 88d4d5ff2f
Tree-SHA512: 3242a24e22313aed97eee32a520bfcb1c17495ba32a2b8e06a5e151e2611320e2da5ef35b572d84623af0a49a210d2f9377a2531250868d1a0ccf3e144352a97
1c1467f51b i2p: cancel the Accept() method if waiting on the socket errors (Vasil Dimov)
Pull request description:
If `Sock::Wait()` fails, then cancel the `Accept()` method.
Not checking the return value may cause an uninitialized read a few lines below when we read the `occurred` variable.
[Spotted](https://github.com/bitcoin/bitcoin/pull/21630#issuecomment-814765659) by MarcoFalke, thanks!
ACKs for top commit:
laanwj:
Code review ACK 1c1467f51b
practicalswift:
cr ACK 1c1467f51b: patch looks correct and agree with laanwj that `[[nodiscard]]` can be taken in a follow-up PR :)
Tree-SHA512: 57fa8a03a4e055999e23121cd9ed1566a585ece0cf68b74223d8c902804cb6890218c9356d60e0560ccacc6c8542a526356c226ebd48e7b299b4572be312d49b
725d7ae049 Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b467 Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090bd6 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc507 Add MissingDataBehavior and make TransactionSignatureChecker handle it (Pieter Wuille)
Pull request description:
Currently we have 2 levels of potentially-missing data in the transaction signature hashes:
* P2WPKH/P2WSH hashes need the spent amount
* P2TR hashes need all spent outputs (amount + scriptPubKey)
Missing amounts are treated as -1 (thus leading to unexpected signature failures), while missing outputs in P2TR validation cause assertion failure. This is hard to extend for signing support, and also quite ugly in general.
In this PR, an explicit configuration option to {Mutable,}TransactionSignatureChecker is added (MissingDataBehavior enum class) to either select ASSERT_FAIL or FAIL. Validation code passes ASSERT_FAIL (as at validation time all data should always be passed, and anything else is a serious bug in the code), while signing code uses FAIL.
The existence of the ASSERT_FAIL option is really just an abundance of caution. Always using FAIL should be just fine, but if there were for some reason a code path in consensus code was introduced that misses certain data, I think we prefer as assertion failure over silently introducing a consensus change.
Potentially useful follow-ups (not for this PR, in my preference):
* Having an explicit script validation error code for missing data.
* Having a MissingDataBehavior::SUCCEED option as well, for use in script/sign.cpp DataFromTransaction (if a signature is present in a witness, and we don't have enough data to fully validate it, we should probably treat it as valid and not touch it).
ACKs for top commit:
sanket1729:
reACK 725d7ae049
Sjors:
ACK 725d7ae049
achow101:
re-ACK 725d7ae049
benthecarman:
ACK 725d7ae049
fjahr:
Code review ACK 725d7ae049
Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
