32fa49a184 make ParseOutputType return a std::optional<OutputType> (fanquake)
Pull request description:
Similar to #22220. Skipped using `auto` here for the same reasons outlined in that PR.
ACKs for top commit:
jnewbery:
utACK 32fa49a184
jonatack:
Code review ACK 32fa49a184 and debian clang 13 debug build is clean / unit tests locally are green
MarcoFalke:
review ACK 32fa49a184🍢
Tree-SHA512: 7752193117669b800889226185d49d164395697853828f8acb568f07651789bc5b2cddc45555957450353886e46b9a1e13c77a5e730a14c6ee621fabc8dc3d10
3f7250b328 [test] Use the new endpoint to improve tests (Amiti Uttarwar)
3893da06db [RPC] Add field to getpeerinfo to indicate if addr relay is enabled (Amiti Uttarwar)
0980ca78cd [test] Test that we intentionally select addr relay peers. (Amiti Uttarwar)
c061599e40 [net_processing] Remove RelayAddrsWithPeer function (Amiti Uttarwar)
201e496481 [net_processing] Introduce new field to indicate if addr relay is enabled (Amiti Uttarwar)
1d1ef2db7e [net_processing] Defer initializing m_addr_known (Amiti Uttarwar)
6653fa3328 [test] Update p2p_addr_relay test to prepare (Amiti Uttarwar)
2fcaec7bbb [net_processing] Introduce SetupAddressRelay (Amiti Uttarwar)
Pull request description:
This PR builds on the test refactors extracted into #22306 (first 5 commits).
This PR aims to reduce addr blackholes. When we receive an `addr` message that contains 10 or less addresses, we forward them to 1-2 peers. This is the main technique we use for self advertisements, so sending to peers that wouldn't relay would effectively "blackhole" the trickle. Although we cannot prevent this in a malicious case, we can improve it for the normal, honest cases, and reduce the overall likelihood of occurrence. Two known cases where peers would not participate in addr relay are if they have connected to you as a block-relay-only connection, or if they are a light client.
This implementation defers initialization of `m_addr_known` until it is needed, then uses its presence to decide if the peer is participating in addr relay. For outbound (not block-relay-only) peers, we initialize the filter before sending the initial self announcement when processing their `version` message. For inbound peers, we initialize the filter if/when we get an addr related message (`ADDR`, `ADDRV2`, `GETADDR`). We do NOT initialize the filter based on a `SENDADDRV2` message.
To communicate about these changes beyond bitcoin core & to (try to) ensure that no other software would be disrupted, I have:
- Posted to the [mailing list](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-April/018784.html)
- Researched other open source clients to confirm compatibility, opened issues in all the projects & documented in https://github.com/bitcoin/bitcoin/pull/21528#issuecomment-809906430. Many have confirmed that this change would not be problematic.
- Raised as topic during [bitcoin-core-dev meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-03-25.html#l-954)
- Raised as topic during [bitcoin p2p meeting](https://www.erisian.com.au/bitcoin-core-dev/log-2021-04-20.html#l-439)
ACKs for top commit:
jnewbery:
reACK 3f7250b328
glozow:
ACK 3f7250b328
ajtowns:
utACK 3f7250b328
Tree-SHA512: 29069282af684c1cd37d107c395fdd432dcccb11626f3c2dabfe92fdc4c85e74c7c4056fbdfa88017fec240506639b72ac6c311f8ce7c583112eb15f47e421af
f685a13bef doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f08 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef
LarryRuane:
Code review, test ACK f685a13bef
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
779e638ca9 coinstats: Add comments for new coinstatsindex values (Fabian Jahr)
5b3d4e724f Index: Improve logging in coinstatsindex (Fabian Jahr)
d4356d4e48 rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo (Fabian Jahr)
a5f6791139 rpc: Add missing gettxoutsetinfo help docs (Fabian Jahr)
01386bfd88 Index: Return early from failed coinstatsindex init (Fabian Jahr)
1e3842385b index: Use batch writing in coinstatsindex WriteBlock (Fabian Jahr)
fb65dde147 scripted-diff: Fix coinstats data member names (Fabian Jahr)
8ea8c927ac index: Avoid unnecessary type casts in coinstatsindex (Fabian Jahr)
Pull request description:
This is a collection of smaller follow-ups to #19521, addressing several post-merge review comments.
ACKs for top commit:
Sjors:
re-utACK 779e638ca9
jonatack:
re-ACK 779e638ca9 diff since last review involves doc changes only; rebased to current master and verified clean debug build/no silent conflicts, unit tests, and feature_coinstatsindex functional test
laanwj:
Code review ACK 779e638ca9
Talkless:
re-utACK 779e638ca9 after cosmetic changes.
Tree-SHA512: cb0d038d230c582d7fe3041c89b1e04d39971fab3739d540c609cf826754c6c513b12ded08ac92180aec7a9d7a70114ece50357bd1a902de4adaae9f30b8d699
During initial sync after startup the gettxoutsetinfo RPC will still return an error while catching up. However, after the initial sync the index will not error immediately anymore when it's in the process of syncing to the tip while being called. Instead it will block until synced and then return the response.
78f4c8b98e prefer to use txindex if available for GetTransaction (Jameson Lopp)
Pull request description:
Fixes#22382
Motivation: prevent excessive disk reads if txindex is enabled.
Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?
ACKs for top commit:
jonatack:
ACK 78f4c8b98e
theStack:
Code review ACK 78f4c8b98e
LarryRuane:
tACK 78f4c8b98e
luke-jr:
utACK 78f4c8b98e
jnewbery:
utACK 78f4c8b98e
rajarshimaitra:
Code review ACK 78f4c8b98e
lsilva01:
Code Review ACK and Tested ACK 78f4c8b98e on Ubuntu 20.04
Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0
2b19f3443e RPC/blockchain: getblockchaininfo: Include versionbits signalling details during LOCKED_IN (Luke Dashjr)
Pull request description:
While the signal has no effect during `LOCKED_IN`, the bit is still defined and recommended for measuring uptake. Makes sense to expose statistics too.
ACKs for top commit:
prayank23:
ACK 2b19f3443e
Sjors:
tACK 2b19f34
theStack:
Tested ACK 2b19f3443e
MarcoFalke:
review-only ACK 2b19f3443e
Tree-SHA512: a9bb5adb21992586119cbb5f87e5348eabcab11d5a3bf769b00b69e466589a669846e503f8384fa8927fd77da0c2d64a54f13a7a55a62980046d70f8255ddf47
20edf4bcf6 rpc: Return block time in getblockchaininfo (João Barbosa)
Pull request description:
Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.
ACKs for top commit:
naumenkogs:
ACK 20edf4bcf6
theStack:
re-ACK 20edf4bcf6
0xB10C:
ACK 20edf4bcf6
kristapsk:
ACK 20edf4bcf6
Zero-1729:
re-ACK 20edf4bcf6
Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
a4bcd687c9 Improve tests using statistics (John Newbery)
f424d601e1 Add logging and addr rate limiting statistics (Pieter Wuille)
b4ece8a1cd Functional tests for addr rate limiting (Pieter Wuille)
5648138f59 Randomize the order of addr processing (Pieter Wuille)
0d64b8f709 Rate limit the processing of incoming addr messages (Pieter Wuille)
Pull request description:
The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that's orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.
This PR implements a [token bucket](https://en.wikipedia.org/wiki/Token_bucket) based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers' self-announcement.
ACKs for top commit:
laanwj:
ACK a4bcd687c9
vasild:
ACK a4bcd687c9
jnewbery:
ACK a4bcd687c9
jonatack:
ACK a4bcd687c9
Tree-SHA512: b757de76ad78a53035b622944c4213b29b3b55d3d98bf23585afa84bfba10808299d858649f92269a16abfa75eb4366ea047eae3216f7e2f6d3c455782a16bea
3c4c8e79ba build: Add -Werror=implicit-fallthrough compile flag (Hennadii Stepanov)
014110c47d Use C++17 [[fallthrough]] attribute, and drop -Wno-implicit-fallthrough (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
fanquake:
ACK 3c4c8e79ba - looks ok to me now. Checked that warnings occur in our code & leveldb by removing a `[[fallthrough]]` or `FALLTHROUGH_INTENDED`.
jarolrod:
ACK 3c4c8e79ba
theStack:
ACK 3c4c8e79ba
Tree-SHA512: 4dce91f0f26b8a3de09bd92bb3d7e1995e078e3a8b3ff861c4fbf6c0b32b2327d063633b07b89c4aa94a1141d7f78d46d9d43ab8df865273e342693ad30645b6
fa5658ed07 Use DeploymentEnabled to hide VB deployments (MarcoFalke)
fa11fecf0d doc: Move buried deployment doc to the enum that enumerates them (MarcoFalke)
Pull request description:
Plus a doc commit.
ACKs for top commit:
jnewbery:
utACK fa5658ed07
ajtowns:
utACK fa5658ed07
Tree-SHA512: 2aeceee0674feb603d76656eff40695b7d7305de309f837bbb6a8c1dbb1d0b962b741f06ab7b9a8b1dbd1964c9c0c9aa5dc9588fd8e6d896e620b69e08eedbaa
Rename BIP9SoftForkPushBack and BuriedSoftForkPushBack to SoftForkPushBack
and have the compiler figure out which one to use based on the deployment
type. Avoids the need to update the file when burying a deployment.
Adds support for versionbits deployments to DeploymentEnabled,
DeploymentActiveAfter and DeploymentActiveAt. Also moves versionbitscache
from validation to deploymentstatus.
Provides DeploymentEnabled, DeploymentActiveAt, and DeploymentActiveAfter
helpers for checking the status of buried deployments. Can be overloaded
so the same syntax works for non-buried deployments, allowing future
soft forks to be changed from signalled to buried deployments without
having to touch the implementation code.
Replaces IsWitnessEnabled and IsScriptWitnessEnabled.
fa0d9211ef refactor: Remove chainparams arg from CChainState member functions (MarcoFalke)
fa38947125 refactor: Remove ::Params() global from inside CChainState member functions (MarcoFalke)
Pull request description:
The `::Params()` global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.
Fix all issues by simply using a member variable that points to the right params.
(Can be reviewed with `--word-diff-regex=.`)
ACKs for top commit:
jnewbery:
ACK fa0d9211ef
kiminuo:
utACK fa0d9211
theStack:
ACK fa0d9211ef🍉
Tree-SHA512: 44676b19c9ed471ccb536331d3029bad192d7d50f394fd7b8527ec431452aeec8c4494164b9cf8e16e0123c4463b16be864366c6b599370032c17262625a0356
754f134a50 wallet: Add error message to GetReservedDestination (Andrew Chow)
87a0e7a3b7 Disallow bech32m addresses for legacy wallet things (Andrew Chow)
6dbe4d1072 Use BECH32M for tr() desc, WitV1Taproot, and WitUnknown CTxDests (Andrew Chow)
699dfcd8ad Opportunistically use bech32m change addresses if available (Andrew Chow)
0262536c34 Add OutputType::BECH32M (Andrew Chow)
177c15d2f7 Limit LegacyScriptPubKeyMan address types (Andrew Chow)
Pull request description:
Currently bech32m addresses are classfied as bech32. Because bech32m is incompatible with bech32, we need to define a new `OutputType` for it so that it can be handled correctly. This PR adds `OutputType::BECH32M`, updates all of the relevant `OutputType` classifications, and handle requests for bech32m addresses. There is now a `bech32m` address type string that can be used.
* `tr()` descriptors now report their output type as `OutputType::BECH32M`. `WtinessV1Taproot` and `WitnessUnknown` are also classified as `OutputType::BECH32M`.
* Bech32m addresses are completely disabled for legacy wallets. They cannot be imported (explicitly disallowed in `importaddress` and `importmulti`), will not be created when getting all destinations for a pubkey, and will not be added with `addmultisigaddress`. Additional protections have been added to `LegacyScriptPubKeyMan` to disallow attempting to retrieve bech32m addresses.
* Since Taproot multisigs are not implemented yet, `createmultisig` will also disallow the bech32m address type.
* As Taproot is not yet active, `DescriptorScriptPubKeyMan` cannot and will not create a `tr()` descriptor. Protections have been added to make sure this cannot occur.
* The change address type detection algorithm has been updated to return `bech32m` when there is a segwit v1+ output script and the wallet has a bech32m `ScriptPubKeyMan`, falling back to bech32 if one is not available.
ACKs for top commit:
laanwj:
re-review ACK 754f134a50
Sjors:
re-utACK 754f134: only change is switching to `bech32m` in two `wallet_taproot.py` test cases.
fjahr:
re-ACK 754f134a50
jonatack:
ACK 754f134a50
Tree-SHA512: 6ea90867d3631d0d438e2b08ce6ed930f37d01323224661e8e38f183ea5ee2ab65b5891394a3612c7382a1aff907b457616c6725665a10c320174017b998ca9f
7ad414f4bf doc: add comment about CCoinsViewDBCursor constructor (James O'Beirne)
0f8a5a4dd5 move-only(ish): don't expose CCoinsViewDBCursor (James O'Beirne)
615c1adfb0 refactor: wrap CCoinsViewCursor in unique_ptr (James O'Beirne)
Pull request description:
I tripped over this one for a few hours at the beginning of the week, so I've sort of got a personal vendetta against `CCoinsView::Cursor()` returning a raw pointer.
Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (`Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.`).
This is a pretty simple change.
Related to: https://github.com/bitcoin/bitcoin/issues/21766
See also: https://github.com/google/leveldb/issues/142#issuecomment-414418135
ACKs for top commit:
MarcoFalke:
review ACK 7ad414f4bf🔎
jonatack:
re-ACK 7ad414f4bf modulo suggestion
ryanofsky:
Code review ACK 7ad414f4bf. Two new commits look good and thanks for clarifying constructor comment
Tree-SHA512: 6471d03e2de674d84b1ea0d31e25f433d52aa1aa4996f7b4aab1bd02b6bc340b15e64cc8ea07bbefefa3b5da35384ca5400cc230434e787c30931b8574c672f9
We don't want the legacy wallet to ever have bech32m addresses so don't
allow importing them. This includes addmultisigaddress as that is a
legacy wallet only RPC
Additionally, bech32m multisigs are not available yet, so disallow them
in createmultisig.
2667366aaa tests: check derivation of P2TR (Pieter Wuille)
7cedafc541 Add tr() descriptor (derivation only, no signing) (Pieter Wuille)
90fcac365e Add TaprootBuilder class (Pieter Wuille)
5f6cc8daa8 Add XOnlyPubKey::CreateTapTweak (Pieter Wuille)
2fbfb1becb Make consensus checking of tweaks in pubkey.* Taproot-specific (Pieter Wuille)
a4bf84039c Separate WitnessV1Taproot variant in CTxDestination (Pieter Wuille)
41839bdb89 Avoid dependence on CTxDestination index order (Pieter Wuille)
31df02a070 Change Solver() output for WITNESS_V1_TAPROOT (Pieter Wuille)
4b1cc08f9f Make XOnlyPubKey act like byte container (Pieter Wuille)
Pull request description:
This is a subset of #21365, to aide review.
This adds support `tr(KEY)` or `tr(KEY,SCRIPT)` or `tr(KEY,{{S1,{{S2,S3},...}},...})` descriptors, describing Taproot outputs with specified internal key, and optionally any number of scripts, in nested groups of 2 inside `{`/`}` if there are more than one. While it permits importing `tr(KEY)`, anything beyond that is just laying foundations for more features later.
Missing:
* Signing support (see #21365)
* Support for more interesting scripts inside the tree (only `pk(KEY)` is supported for now). In particular, a multisig policy based on the new `OP_CHECKSIGADD` opcode would be very useful.
* Inferring `tr()` descriptors from outputs (given sufficient information).
* `getaddressinfo` support.
* MuSig support. Standardizing that is still an ongoing effort, and is generally kind of useless without corresponding PSBT support.
* Convenient ways of constructing descriptors without spendable internal key (especially ones that arent't trivially recognizable as such).
ACKs for top commit:
Sjors:
utACK 2667366 (based on https://github.com/bitcoin/bitcoin/pull/21365#issuecomment-846945215 review, plus the new functional test)
achow101:
Code Review ACK 2667366aaa
lsilva01:
Tested ACK 2667366aaa
meshcollider:
utACK 2667366aaa
Tree-SHA512: 61046fef22c561228338cb178422f0b782ef6587ec8208d3ce2bd07afcff29a664b54b35c6b01226eb70b6540b43f6dd245043d09aa6cb6db1381b6042667e75
