Commit 181a1207 introduced an initialization order bug: CAddrMan's
m_asmap must be set before deserializing peers.dat. Restore that
ordering.
review hint: use
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
a38137479bf5e25bf65a62e46b81fc43fb3df75c net: do not advertise address where nobody is listening (Jadi)
Pull request description:
If the bitcoind starts when listen=0 but listenonion=1, the daemon will
advertise its onion address but nothing is listening for it.
This update will enforce listenonion=0 when the listen is 0.
ACKs for top commit:
vasild:
ACK a38137479bf5e25bf65a62e46b81fc43fb3df75c
jarolrod:
ACK a38137479bf5e25bf65a62e46b81fc43fb3df75c
amitiuttarwar:
ACK a38137479bf5e25bf65a62e46b81fc43fb3df75c
Tree-SHA512: e84a0a9a51f2217edf35d06c6cd9085d1e664452655ba92027195a1e88ba081d157310c84e9709a99ce5d46c94f231477ca2d36f010648b0c8b4f2a737d54e5d
Now that we manage the lifespan of node.addrman, we can just reset
node.addrman to a newly initialized CAddrMan if parsing peers.dat
fails, eliminating the possibility that Clear() leaves some old state
behind.
Use interfaces::Init::make* methods instead of interfaces::Make*
functions, so interfaces can be constructed differently in different
executables without having to change any code. (So for example
bitcoin-gui can make an interfaces::Node pointer that communicates with
a bitcoin-node subprocess, while bitcoin-qt can make an interfaces::Node
pointer that starts node code in the same process.)
Currently addrman consistency checks are a compile time option, and are not
enabled in our CI. It's unlikely anyone is running these consistency checks.
Make them a runtime option instead, where users can enable addrman
consistency checks every n operations (similar to mempool tests). Update
the addrman unit tests to do internal consistency checks every 100
operations (checking on every operations causes the test runtime to
increase by several seconds).
Also assert on a failed addrman consistency check to terminate program
execution.
703b1e612a4bd4521e20ae21eb8fb7c19f4ef942 Close minor startup race between main and scheduler threads (Larry Ruane)
Pull request description:
This is a low-priority bug fix. The scheduler thread runs `CheckForStaleTipAndEvictPeers()` every 45 seconds (EXTRA_PEER_CHECK_INTERVAL). If its first run happens before the active chain is set up (`CChain::SetTip()`), `bitcoind` will assert:
```
(...)
2021-07-28T22:16:49Z init message: Loading block index…
bitcoind: validation.cpp:4968: CChainState& ChainstateManager::ActiveChainstate() const: Assertion `m_active_chainstate' failed.
Aborted (core dumped)
```
I ran into this while using the debugger to investigate an unrelated problem. Single-stepping through threads with a debugger can cause the relative thread execution timing to be very different than usual. I don't think any automated tests are needed for this PR. I'll give reproduction steps in the next PR comment.
ACKs for top commit:
MarcoFalke:
cr ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942
tryphe:
tested ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942
0xB10C:
ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942
glozow:
code review ACK 703b1e612a4bd4521e20ae21eb8fb7c19f4ef942 - it makes sense to me to start peerman's background tasks here, after `chainstate->LoadChainTip()` and `node.connman->Start()` have been called.
Tree-SHA512: 9316ad768cba3b171f62e2eb400e3790af66c47d1886d7965edb38d9710fc8c8f8e4fb38232811c9346732ce311d39f740c5c2aaf5f6ca390ddc48c51a8d633b
82b6f89819e55af26f5264678e0f93052934bcb3 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64c1dc1a56a3bcb6b5e2b4fb95e8b29f [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed5689ea72e9a1618f14551775246f6361 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bdcf863af160c0426e3a22028ca8259a [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2573e0297c9b0e095c2a0868929b08b [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a928ffacca240fadbf1747a42d5ba256 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361552eeecd100cee8cc40d4cd5a3aa27 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c0871977839c28636eff975748182888299cd2b [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)
Pull request description:
This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).
The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.
The tests include:
* parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
* the delay before querying DNS seeds depending on how many addresses are in the addrman
* the behavior of `-forcednsseed`
* skipping DNS querying if we have outbound full relay connections & not block-relay-only connections
Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽
ACKs for top commit:
mzumsande:
ACK 82b6f89819e55af26f5264678e0f93052934bcb3
jnewbery:
reACK 82b6f89819e55af26f5264678e0f93052934bcb3
Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
Don't schedule class PeerManagerImpl's background tasks from its
constructor, but instead do that from a separate method,
StartScheduledTasks(), that can be called later at the end of startup,
after other things, such as the active chain, are initialzed.
-dnsseed determines whether we run ThreadDNSAddressSeed and potentially query
the DNS seeds for addresses. -forcednsseed tells the node to force querying the
DNS seeds even if we have sufficient addresses or current connections.
This commit disallows starting up with explicitly conflicting parameters.
a806647d260132a00cd633160040625c7dd17803 [validation] Always include merkle root in coinbase commitment (Dhruv Mehta)
189128c220190a588500b8e74ee7ae47671b9558 [validation] Set witness script flag with p2sh for blocks (Dhruv Mehta)
ac82b99db77ec843af82dcdf040dfdbc98c8ff26 [p2p] remove redundant NODE_WITNESS checks (Dhruv Mehta)
6f8b198b8256a6703a6f5e592dfa77fa024a7035 [p2p] remove unused segwitheight=-1 option (Dhruv Mehta)
eba5b1cd6460c98e75d0422bd394e12af7f11e4c [test] remove or move tests using `-segwitheight=-1` (Dhruv Mehta)
Pull request description:
Builds on #21009 and makes progress on remaining items in #17862
Removing `RewindBlockIndex()` in #21009 allows the following:
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- move `test_upgrade_after_activation()` out of `p2p_segwit.py` reducing runtime
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
mzumsande:
Code-Review ACK a806647d260132a00cd633160040625c7dd17803
laanwj:
Code review ACK a806647d260132a00cd633160040625c7dd17803, nice cleanup
jnewbery:
utACK a806647d260132a00cd633160040625c7dd17803
theStack:
ACK a806647d260132a00cd633160040625c7dd17803
Tree-SHA512: 73e1a69d1d7eca1f5c38558ec6672decd0b60b16c2ef6134df6f6af71bb159e6eea160f9bb5ab0eb6723c6632d29509811e29469d0d87abbe9b69a2890fbc73e
ceb7b35a39145717e2d9d356fd382bd1f95d2a5a refactor: move UpdateTip into CChainState (James O'Beirne)
4abf0779d6594e97222279110c328b75b5f3db7b refactor: no mempool arg to GetCoinsCacheSizeState (James O'Beirne)
46e3efd1e4ae2f058ecfffdaee7e882c4305eb35 refactor: move UpdateMempoolForReorg into CChainState (James O'Beirne)
617661703ac29e0744f21de74501d033fdc53ff6 validation: make CChainState::m_mempool optional (James O'Beirne)
Pull request description:
Make `CChainState::m_mempool` optional by making it a pointer instead of a reference. This will allow a simplification to assumeutxo semantics (see https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905) and help facilitate the `-nomempool` option.
ACKs for top commit:
jnewbery:
ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a
naumenkogs:
ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a
ryanofsky:
Code review ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a (just minor style and test tweaks since last review)
lsilva01:
Code review ACK and tested on Signet ACK ceb7b35a39
MarcoFalke:
review ACK ceb7b35a39145717e2d9d356fd382bd1f95d2a5a 😌
Tree-SHA512: cc445ad33439d5918cacf80a6354eea8f3d33bb7719573ed5b970fad1a0dab410bcd70be44c862b8aba1b71263b82d79876688c553e339362653dfb3d8ec81e6
Since we now have multiple chainstate objects, only one of them is active at any given
time. An active chainstate has a mempool, but there's no point to others having one.
This change will simplify proposed assumeutxo semantics. See the discussion here:
https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
4101ec9d2e05a35c35f587a28f1feee6cebcc61b doc: mention that we enforce port=0 in I2P (Vasil Dimov)
e0a2b390c144e123e2fc8a289fdff36815476964 addrman: reset I2P ports to 0 when loading from disk (Vasil Dimov)
41cda9d075ebcab1dbb950160ebe9d0ba7b5745e test: ensure I2P ports are handled as expected (Vasil Dimov)
4f432bd738c420512a86a51ab3e00323f396b89e net: do not connect to I2P hosts on port!=0 (Vasil Dimov)
1f096f091ebd88efb18154b8894a38122c39624f net: distinguish default port per network (Vasil Dimov)
aeac3bce3ead1f24ca782079ef0defa86fd8cb98 net: change I2P seeds' ports to 0 (Vasil Dimov)
38f900290cc3a839e99bef13474d35e1c02e6b0d net: change assumed I2P port to 0 (Vasil Dimov)
Pull request description:
_This is an alternative to https://github.com/bitcoin/bitcoin/pull/21514, inspired by https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-815049933. They are mutually exclusive. Just one of them should be merged._
Change assumed ports for I2P to 0 (instead of the default 8333) as this is closer to what actually happens underneath with SAM 3.1 (https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-812632520, https://github.com/bitcoin/bitcoin/pull/21514#issuecomment-816564719).
Don't connect to I2P peers with advertised port != 0 (we don't specify a port to our SAM 3.1 proxy and it always connects to port = 0).
Note, this change:
* Keeps I2P addresses with port != 0 in addrman and relays them to others via P2P gossip. There may be non-bitcoin-core-22.0 peers using SAM 3.2 and for them such addresses may be useful.
* Silently refuses to connect to I2P hosts with port != 0. This is ok for automatically chosen peers from addrman. Not so ok for peers provided via `-addnode` or `-connect` - a user who specifies `foo.b32.i2p:1234` (non zero port) may wonder why "nothing is happening".
Fixes#21389
ACKs for top commit:
laanwj:
Code review ACK 4101ec9d2e05a35c35f587a28f1feee6cebcc61b
jonatack:
re-ACK 4101ec9d2e05a35c35f587a28f1feee6cebcc61b per `git range-diff efff9c3 0b0ee03 4101ec9`, built with DDEBUG_ADDRMAN, did fairly extensive testing on mainnet both with and without a peers.dat / -dnsseeds=0 to test boostrapping.
Tree-SHA512: 0e3c019e1dc05e54f559275859d3450e0c735596d179e30b66811aad9d5b5fabe3dcc44571e8f7b99f9fe16453eee393d6e153454dd873b9ff14907d4e6354fe
2feec3ce3130961f98ceb030951d0e46d3b9096c net: don't bind on 0.0.0.0 if binds are restricted to Tor (Vasil Dimov)
Pull request description:
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
ACKs for top commit:
laanwj:
Code review ACK 2feec3ce3130961f98ceb030951d0e46d3b9096c
jonatack:
utACK 2feec3ce3130961f98ceb030951d0e46d3b9096c per `git diff a004833 2feec3c`
hebasto:
ACK 2feec3ce3130961f98ceb030951d0e46d3b9096c, tested on Linux Mint 20.1 (x86_64):
Tree-SHA512: a04483af601706da928958b92dc560f9cfcc78ab0bb9d74414636eed1c6f29ed538ce1fb5a17d41ed82c9c9a45ca94899d0966e7ef93da809c9bcdcdb1d1f040
* When accepting an I2P connection, assume the peer has port 0 instead
of the default 8333 (for mainnet). It is not being sent to us, so we
must assume something.
* When deriving our own I2P listen CService use port 0 instead of the
default 8333 (for mainnet). So that we later advertise it to peers
with port 0.
In the I2P protocol SAM 3.1 and older (we use 3.1) ports are not used,
so they are irrelevant. However in SAM 3.2 and newer ports are used and
from the point of view of SAM 3.2, a peer using SAM 3.1 seems to have
specified port=0.
The semantic of `-bind` is to restrict the binding only to some address.
If not specified, then the user does not care and we bind to `0.0.0.0`.
If specified then we should honor the restriction and bind only to the
specified address.
Before this change, if no `-bind` is given then we would bind to
`0.0.0.0:8333` and to `127.0.0.1:8334` (incoming Tor) which is ok -
the user does not care to restrict the binding.
However, if only `-bind=addr:port=onion` is given (without ordinary
`-bind=`) then we would bind to `addr:port` _and_ to `0.0.0.0:8333` in
addition.
Change the above to not do the additional bind: if only
`-bind=addr:port=onion` is given (without ordinary `-bind=`) then bind
to `addr:port` (only) and consider incoming connections to that as Tor
and do not advertise it. I.e. a Tor-only node.
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.
fa0d9211ef87a682573aaae932c0c440acbcb8a8 refactor: Remove chainparams arg from CChainState member functions (MarcoFalke)
fa389471251f043ec25e7b01e59b37d3b921ce54 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 fa0d9211ef87a682573aaae932c0c440acbcb8a8
kiminuo:
utACK fa0d9211
theStack:
ACK fa0d9211ef87a682573aaae932c0c440acbcb8a8 🍉
Tree-SHA512: 44676b19c9ed471ccb536331d3029bad192d7d50f394fd7b8527ec431452aeec8c4494164b9cf8e16e0123c4463b16be864366c6b599370032c17262625a0356
Save the banlist in `banlist.json` instead of `banlist.dat`.
This makes it possible to store Tor v3 entries in the banlist on disk
(and any other addresses that cannot be serialized in addrv1 format).
Only read `banlist.dat` if it exists and `banlist.json` does not
exist (first start after an upgrade).
Supersedes https://github.com/bitcoin/bitcoin/pull/20904
Resolves https://github.com/bitcoin/bitcoin/issues/19748
6f994882deafe62e97f0a889d8bdb8c96dcf913d validation: Farewell, global Chainstate! (Carl Dong)
972c5166ee685447a6d4bf5e501b07a0871fba85 qt/test: Reset chainman in ~ChainstateManager instead (Carl Dong)
6c3b5dc0c13c3ac8c6e86298f924abe99d8d6bd1 scripted-diff: tree-wide: Remove all review-only assertions (Carl Dong)
3e82abb8dd7e21ec918966105648be7ae077fd8c tree-wide: Remove stray review-only assertion (Carl Dong)
f323248aba5088c9630e5cdfe5ce980f21633fe8 qt/test: Use existing chainman in ::TestGUI (can be scripted-diff) (Carl Dong)
6c15de129cd645bf0547cb184003fae131b95b83 scripted-diff: wallet/test: Use existing chainman (Carl Dong)
ee0ab1e959e0e75e04d87fabae8334ad4656f3e5 fuzz: Initialize a TestingSetup for test_one_input (Carl Dong)
0d61634c066a7102d539e85e2b1a4ca15be9660a scripted-diff: test: Use existing chainman in unit tests (Carl Dong)
e197076219e986ede6cf924e0ea36bd723503b2d test: Pass in CoinsTip to ValidateCheckInputsForAllFlags (Carl Dong)
4d99b61014ba26eb1f3713df5528d2804edff165 test/miner_tests: Pass in chain tip to CreateBlockIndex (Carl Dong)
f0dd5e6bb4b16e69d35b648b7ef973a732229873 test/util: Use existing chainman in ::PrepareBlock (Carl Dong)
464c313e304cef04a82e14f736e3c44ed5604a4e init: Use existing chainman (Carl Dong)
Pull request description:
Based on: #21767
à la Mr. Sandman
```
Mr. Chainman, bring me a tip (bung, bung, bung, bung)
Make it the most work that I've ever seen (bung, bung, bung, bung)
Rewind old tip till we're at the fork point (bung, bung, bung, bung)
Then tell it that it's time to call Con-nectTip
Chainman, I'm so alone (bung, bung, bung, bung)
No local objects to call my own (bung, bung, bung, bung)
Please make sure I have a ref
Mr. Chainman, bring me a tip!
```
This is the last bundle in the #20158 series. Thanks everyone for their diligent review.
I would like to call attention to https://github.com/bitcoin/bitcoin/issues/21766, where a few leftover improvements were collated.
- Remove globals:
- `ChainstateManager g_chainman`
- `CChainState& ChainstateActive()`
- `CChain& ChainActive()`
- Remove all review-only assertions.
ACKs for top commit:
jamesob:
reACK 6f994882de based on the contents of
ariard:
Code Review ACK 6f99488.
jnewbery:
utACK 6f994882deafe62e97f0a889d8bdb8c96dcf913d
achow101:
Code Review ACK 6f994882deafe62e97f0a889d8bdb8c96dcf913d
ryanofsky:
Code review ACK 6f994882deafe62e97f0a889d8bdb8c96dcf913d.
Tree-SHA512: 4052ea79360cf0efd81ad0ee3f982e1d93aab1837dcec75f875a56ceda085de078bb3099a2137935d7cc2222004ad88da94b605ef5efef35cb6bc733725debe6
7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a index: refactor-only: Reuse CChain ref (Carl Dong)
db33cde80fff749c6adff9e91fca5f27f4bb6278 index: Add chainstate member to BaseIndex (Carl Dong)
f4a47a1febfa35ab077f2a841fe31a8cd9618250 bench: Use existing chainman in AssembleBlock (Carl Dong)
91226eb91769aad5a63bc671595e1353a2b2247a bench: Use existing NodeContext in DuplicateInputs (Carl Dong)
e6b4aa6eb53dc555ecab2922af35e7a2572faf4f miner: Pass in chainman to RegenerateCommitments (Carl Dong)
9ecade14252ad1972f668d2d2e4ef44fdfcb944a rest: Add GetChainman function and use it (Carl Dong)
fc1c282845f6b8436d1ea4c68eb3511034c29bea rpc/blockchain: Use existing blockman in gettxoutsetinfo (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
The first 2 commits are fixups addressing review for the last bundle: #21391
NEW note:
1. I have opened #21766 which keeps track of potential improvements where the flaws already existed before the de-globalization work, please post on that issue about these improvements, thanks!
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:
jarolrod:
ACK 7a799c9
ariard:
Code Review ACK 7a799c9
fjahr:
re-ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a
MarcoFalke:
review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a 🌠
ryanofsky:
Code review ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a. Basically no change since last review except fixed rebase conflicts and a new comment about REST Ensure()
jamesob:
conditional ACK 7a799c9c2b8652e780d1fd5e1bf7d05b026c1c1a ([`jamesob/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai`](https://github.com/jamesob/bitcoin/tree/ackr/21767.1.dongcarl.bundle_6_n_prune_g_chai))
Tree-SHA512: 531c00ddcb318817457db2812d9a9d930bc664e58e6f7f1c746350732b031dd624270bfa6b9f49d8056aeb6321d973f0e38e4ff914acd6768edd8602c017d10e
a7a43e8fe85f6247c35d7ff99f36448574f3e34a Factor feefilter logic out (amadeuszpawlik)
c0385f10a133d5d8a4c296e7b7a6d75c9c4eec12 Remove -feefilter option (amadeuszpawlik)
Pull request description:
net: Remove -feefilter option, as it is debug only and isn't used in any tests. Checking this option for every peer on every iteration of the message handler is unnecessary, as described in #21545.
refactor: Move feefilter logic out into a separate `MaybeSendFeefilter(...)` function to improve readability of the already long `SendMessages(...)`. fixes #21545
The configuration option `-feefilter` has been added in 9e072a6e66efbda7d39bf61eded21d2b324323be: _"Implement "feefilter" P2P message"_
According to the [BIP133](https://github.com/bitcoin/bips/blob/master/bip-0133.mediawiki), turning the fee filter off was ment for:
> [...] a node [...] using prioritisetransaction to accept transactions whose actual fee rates might fall below the node's mempool min fee [in order to] disable the fee filter to make sure it is exposed to all possible txid's
`-feefilter` was subsequently set as debug only in #8150, with the motivation that the help message was too difficult to translate.
ACKs for top commit:
jnewbery:
Code review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a
promag:
Code review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a.
MarcoFalke:
review ACK a7a43e8fe85f6247c35d7ff99f36448574f3e34a 🦁
Tree-SHA512: 8ef9a2f255597c0279d3047dcc968fd30fb7402e981b69206d08eed452c705ed568c24e646e98d06eac118eddd09205b584f45611d1c874abf38f48b08b67630
Feefilter option is debug only and it isn't used in any tests, it's wasteful
to check this option for every peer on every iteration of the message handler
loop. refs #21545
792be53d3e9e366b9f6aeee7a1eeb912fa28062e refactor: Replace std::bind with lambdas (Hennadii Stepanov)
a508f718f3e087c96a306399582a85df2e1d53ae refactor: Use appropriate thread constructor (Hennadii Stepanov)
30e44482152488a78f2c495798a75e6f553dc0c8 refactor: Make TraceThread a non-template free function (Hennadii Stepanov)
Pull request description:
This PR does not change behavior.
Its goal is to improve readability and maintainability of the code.
ACKs for top commit:
jnewbery:
utACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e
jonatack:
tACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e
MarcoFalke:
cr ACK 792be53d3e9e366b9f6aeee7a1eeb912fa28062e
Tree-SHA512: a03142f04f370f6bc02bd3ddfa870819b51740fcd028772241d68c84087f95a2d78207cbd5edb3f7c636fcf2d76192d9c59873f8f0af451d3b05c0cf9cf234df
fae196147bae11202c0d54543dc12ba5d92ab0cc doc: Clarify that feerates are per virtual size (MarcoFalke)
fa83e95ac6f318caa38016a08fa4e402c3b05833 scripted-diff: Clarify that feerates are per virtual size (MarcoFalke)
Pull request description:
By implementing segwit, it is already clear that all feerates in Bitcoin Core are denoted in (amount/virtual size). Though, there is inconsistency, as some places use kvB, some use kB. Thus, replace all with "kvB".
See also commit 6da3afbaee5809ebf6d88efaa3958c505c2d71c7, which did the replacement for wallet RPCs.
ACKs for top commit:
ryanofsky:
Code review ACK fae196147bae11202c0d54543dc12ba5d92ab0cc. Checked instances where units were being added in the second commit and they all looked right.
Tree-SHA512: ab70d13cde7d55c1ac931bddc2b45aa218fc75ef46cb6ea9e5a30b1d4dbf27889c2b6357299a6c5427912443a46ec3592a4809dae335e03162bd2120a0f7f8ad
d66f283ac07edce432b964f7f814631f5a5bc33b scripted-diff: Replace three dots with ellipsis in the UI strings (Hennadii Stepanov)
Pull request description:
This PR is split from #21463.
The change was suggested on [Transifex.com](https://www.transifex.com/bitcoin/bitcoin/), and it does not touch `LogPrint` and `LogPrintf` calls.
The only comment on #21463 [was](9030e4b5a6 (r597220100)):
> Mind that these messages also end up in the log. In principle the log is already UTF-8 (as are all strings and text in bitcoind). But, just noting, that it might make browsing the log a less pleasant experience on systems with misconfigured locale like some BSDs by default.
ACKs for top commit:
laanwj:
ACK d66f283ac07edce432b964f7f814631f5a5bc33b
Tree-SHA512: 5ab1cb3160f3f996f1ad7d7486662da3eb7f06a857f4a1874963ce10caed5b86b0ad6151b1b9ebeb2b8aa5f0c85efad3b768ea9cafe5db86f78f88912b756d1e
b4fcbcfb49461b96bc72fb64d6152de7c5ce00de doc: update -maxconnections config option help (Jon Atack)
79685a8992ad302833b506cc6d03aab1cc127de0 doc: update -addnode config option help (Jon Atack)
2896c6c4cc6d382d8369c037e274c08dd8e32c69 doc: update addnode rpc help (Jon Atack)
Pull request description:
Since #9319 proposed by Gregory Maxwell and released in v0.14, peers manually added through the `-addnode` config option or using the `addnode` RPC have their own separate limit of 8 connections that does not compete with other inbound or outbound connection usage and is not subject to the limitation imposed by the `-maxconnections` option.
This PR updates the `-addnode` and `-maxconnections` config options and the `addnode` RPC help docs with this information.
`-addnode` config option help
```
$ bitcoind -h | grep -A5 addnode=
-addnode=<ip>
Add a node to connect to and attempt to keep the connection open (see
the addnode RPC help for more info). This option can be specified
multiple times to add multiple nodes; connections are limited to
8 at a time and are counted separately from the -maxconnections
limit.
$ bitcoind -h | grep -A3 maxconnections=
-maxconnections=<n>
Maintain at most <n> connections to peers (default: 125). This limit
does not apply to connections manually added via -addnode or the
addnode RPC, which have a separate limit of 8.
```
`addnode` rpc help
```
$ bitcoin-cli help addnode
addnode "node" "command"
Attempts to add or remove a node from the addnode list.
Or try a connection to a node once.
Nodes added using addnode (or -connect) are protected from DoS disconnection and are not required to be
full nodes/support SegWit as other outbound peers are (though such peers will not be synced from).
Addnode connections are limited to 8 at a time and are counted separately from the -maxconnections limit.
```
ACKs for top commit:
prayank23:
ACK b4fcbcfb49
jarolrod:
ACK b4fcbcfb49461b96bc72fb64d6152de7c5ce00de
Tree-SHA512: b6d69baa6cbf6d53f91bac5b39b549d49db6c95f92ea1bdd3588a6432794a25ac2c8b3c89e2c72bb9097e61f2717c8b5ecc404745d5992b88e523db03200898f
5f96d7d22d8e05876c6fc014e70488699950fe38 rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436be641d7417152adc683192649ba206a test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3cfbd6bce5883f46d8527c6853a86a2 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392aef2689bc106c20925859ede555d082b index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121a30489bc81a1f46dde6a9b19ae4ec1 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c29099635150014ffc9bb0cafa8049ec55a test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576eccb08af80471c7b7b843b189ad6758c0 rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836a71af23d2035d2e2e99ad8b8c340c3 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d689f93e1c7669b0ba7a4994c0206dabd rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30fef3138bb8db46e6865acb9dc2674f8 test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab4d073a26c3c4a02783d5dcd88f6eef test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc125fce6ccd995687fa16572090a5d099 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff323255b32e717d0ce28209ec0abdaa index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21469d6d848ae309edc47f558628221d index: Add Coinstats index (Fabian Jahr)
a8a46c4b3cfda4b95c92a36f8cebd3606377e57d refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd21a87228c18a1661df99fedc1866baf refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a9021dfbb6e17dfa81472f057dacbc34e0 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d8e05876c6fc014e70488699950fe38
jonatack:
Code review re-ACK 5f96d7d22d8e05876c6fc014e70488699950fe38 per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d8e05876c6fc014e70488699950fe38. Light code review ACK 5f96d7d22d8e05876c6fc014e70488699950fe38.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
