586190f0b4740457cb86cba632e3d64e6dfe9b0c rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong)
bc3bd369027273278a0541f3b991eb71de831aa2 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong)
f99913969f92b8b9cef1b83f5ee8e6a9267b4af0 rpc: Remove unnecessary casting of block height (Carl Dong)
6a3d1920209cded0dae52fb9070a3530d9a4e5fd rpc: Tidy up local references (see commit message) (Carl Dong)
038854f31e3511e8bb6e163305cab0a96783d25b rest/rpc: Remove now-unused old Ensure functions (Carl Dong)
6fb65b49f4d393b091479be5a5df5a0a160cf986 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong)
1570c7ee98612366df031bebef9e0468fb57b8a2 rpc: Add renamed EnsureAny*() functions (Carl Dong)
306b1cd3eeb2502904ed4698646d2c86d028aad2 rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong)
d7824acdb9b18fe8f151771a83ccae1681f16c66 rest: Use existing NodeContext (Carl Dong)
3f0893479908ca28d6127c8d0ada30737cb830be rest: Pass in NodeContext to rest_block (Carl Dong)
7be0671b950842fc3a17641a4a21501de0a800b5 rpc/rawtx: Use existing NodeContext (Carl Dong)
60dc05afc6f6388c6f86729a0edd7cb69f1748e0 rpc/mining: Use existing NodeContext (Carl Dong)
d485e815e2b62dc74a485569d08130dc3ef9ff63 rpc/blockchain: Use existing NodeContext (Carl Dong)
d0abf0bf429586e3a5b4c3231fe430dc29695481 rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong)
cced0f46c9133e0fc6211e987421ad1d9be1a399 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 586190f0b4740457cb86cba632e3d64e6dfe9b0c. 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 586190f0b4740457cb86cba632e3d64e6dfe9b0c
MarcoFalke:
review ACK 586190f0b4740457cb86cba632e3d64e6dfe9b0c 🍯
Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
63631beef6a0046390469971adf4500718ab34ad 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 63631beef6a0046390469971adf4500718ab34ad
Tree-SHA512: 06863d1edfb9ad1ca9bcae09cf3f0f47b58bb29d222b70799c3dc059b96452889026e4b99b132782846d9896e3e798d17c7f9406e0e6a0bec1bffc6edb54e9df
549c82ad3a34a885ecca37a5f04c36dfbaa95d17 fuzz: use ConsumeBool() instead of !ConsumeBool() (Vasil Dimov)
29ae1c13a59187119f5b2a38b54dbbec936d8f87 fuzz: split FuzzedSock interface and implementation (Vasil Dimov)
9668e43d8e757c0185b900eb6ee6891a0ba41666 fuzz: make FuzzedSock::Wait() sometimes simulate an occurred event (Vasil Dimov)
0c90ff1429deaa556c0509c13cdd5aef5df9c0d4 fuzz: set errno from FuzzedSock::Wait() if it simulates a failure (Vasil Dimov)
5198a02de4e7a1b0efe28c6095745ce59f7f98c4 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 549c82ad3a34a885ecca37a5f04c36dfbaa95d17: patch looks correct and touches only `src/test/fuzz/`
MarcoFalke:
re-ACK 549c82ad3a34a885ecca37a5f04c36dfbaa95d17 only change is rebase 🎬
Tree-SHA512: 8ba965a8319074ad2ef840219c35c77e37cc79f00fb3926f20ccbf5f58e9616f5a3ac96434ad33996b47d292fa760d5d00a529001ac0d1d254262e5df93f616f
fa40d6a1c47ac7f3dc6c11a2e6642cfef95422c1 test: Reset mocktime in the common setup (MarcoFalke)
fa78590a8fffdfc7e98ddb1f81218f05b1935a0a 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 fa40d6a1c47ac7f3dc6c11a2e6642cfef95422c1
jarolrod:
ACK fa40d6a1c47ac7f3dc6c11a2e6642cfef95422c1
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.
6262182b3f1c9540291fb8de3bf7a785e7113c55 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 6262182b3f1c9540291fb8de3bf7a785e7113c55
Tree-SHA512: e622acb4d01446c3db01adbbbb779038be7247e13f3f4e72c614bc2880c3efd710fd3b189f87abb00f236fa5ddf91f4c215f420ca4eb08a97aaba31593254c3d
ffe33dfbd4c3b11e3475b022b6c1dd077613de79 chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6bcd2c2ce5fea84cf8681013f85a444e7ea versionbits: simplify state transitions (Anthony Towns)
55ac5f568a3b73d6f1ef4654617fb76e8bcbccdf versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6da48040dc7eae46bc7941db48d98a669fd fuzz: test versionbits delayed activation (Anthony Towns)
dd85d5411c1702c8ae259610fe55050ba212e21e tests: test versionbits delayed activation (Anthony Towns)
73d4a706393e6dbd6b6d6b6428f8d3233ac0a2d8 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f6fa205eee5c3b99343988adcb8d320460 tests: clean up versionbits test (Anthony Towns)
593274445004506c921d5d851361aefb3434d744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0a4760c0c0f784029849cb5d21ee088abb 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 ffe33dfbd4c3b11e3475b022b6c1dd077613de79
MarcoFalke:
review ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 💈
achow101:
re-ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
gmaxwell:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
benthecarman:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
Sjors:
ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79
jonatack:
Initial approach ACK ffe33dfbd4c3b11e3475b022b6c1dd077613de79 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
fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d51bb0e25eb3fbd716881fa55527dc7 blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad24d3c9515d3f9c136af4071cbd79055 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3447a3645e7958c7dc4e1946a69cb9c move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a14744e7d7f7746e861aabd9cf938f61 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f99f91603e08ea858f972516f1d669a 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 fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f ([`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 fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f. 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
003929c0d55532038d5bf6fc0ff4a20628710fae 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 003929c0d55532038d5bf6fc0ff4a20628710fae
Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
725d7ae0494d4a45f5a840bbbd19c008a7363965 Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b467330b2c6bb0d44786020c55f1aa75f9 Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090bd619ac85ab35eff376c03136fe4a9f04 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc507bdc716e5236b1d9880e648147e0af9 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 725d7ae0494d4a45f5a840bbbd19c008a7363965
Sjors:
ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
achow101:
re-ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
benthecarman:
ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
fjahr:
Code review ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
This removes the DEFINED->FAILED transition and changes the
STARTED->FAILED transition to only occur if signalling didn't pass the
threshold. This ensures that it is always possible for activation to
occur, no matter what settings are chosen, or the speed at which blocks
are found.
Previously we used deployments that would timeout prior to Bitcoin's
invention, which allowed the deployment to still be activated in unit
tests. This switches those deployments to be truly never active.
This generalises the ComputeBlockVersion test so that it can apply to
any activation parameters we might set, and checks all the parameters
set for each deployment on each chain, to simultaneously ensure that the
deployments we have configured work sensibly, and that the test code
does not suffer bitrot in the event that all interesting deployments
are buried.
The intent here is to allow checking ComputeBlockVersion behaviour with
each deployment, rather than only testdummy on mainnet. This commit does
the trivial refactoring component of that change.
d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec doc: release notes for new listbanned fields (Jarol Rodriguez)
60290d3f5ec8e7e3b8cb1ebae02d5d72f6005184 test: increase listbanned unit test coverage (Jon Atack)
3e978d1a5dbd43f85bd03e759984ab1f209d6e34 rpc: add time_remaining field to listbanned (Jarol Rodriguez)
5456b345312857981cb426712f0665800c682e09 rpc: add ban_duration field to listbanned (Jarol Rodriguez)
c95c61657afd058b46549fb3d65633d7c736f5fc doc: improve listbanned help (Jarol Rodriguez)
dd3c8eaa3399b28dc78a883ff78cbe7cc5c31b5b rpc: swap position of banned_until and ban_created fields (Jarol Rodriguez)
Pull request description:
This PR adds a `ban_duration` and `time_remaining` field to the `listbanned` RPC command. Thanks to jonatack, this PR also expands the `listbanned` test coverage to include these new fields
It's useful to keep track of `ban_duration` as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI `bantablemodel` as part of a follow-up PR. As [suggested](https://github.com/bitcoin/bitcoin/pull/21602#issuecomment-813486134) by jonatack, `time_remaining` is another useful user-centric data point.
Since a ban always expires after its created, the `ban_created` field is now placed before the `banned_until` field. This new ordering is more logical.
This PR also improves the `help listbanned` output by providing additional context to the descriptions of the `address`, `ban_created`, and `banned_until` fields.
**Master: listbanned**
```
[
{
"address": "1.2.3.4/32",
"banned_until": 1617691101,
"ban_created": 1617604701
},
{
"address": "135.181.41.129/32",
"banned_until": 1649140716,
"ban_created": 1617604716
}
]
```
**PR: listbanned**
```
[
{
"address": "1.2.3.4/32",
"ban_created": 1617775773,
"banned_until": 1617862173,
"ban_duration": 86400,
"time_remaining": 86392
},
{
"address": "3.114.211.172/32",
"ban_created": 1617753165,
"banned_until": 1618357965,
"ban_duration": 604800,
"time_remaining": 582184
}
]
```
ACKs for top commit:
jonatack:
re-ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec
hebasto:
ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec, tested on Linux Mint 20.1 (x86_64).
MarcoFalke:
review ACK d3b0b08b0f04d2f1dbebbafd7ab0384dfe045dec 🕙
Tree-SHA512: 5b83ed2483344e546d57e43adc8a1ed7a1fff292124b14c86ca3a1aa2aec8b0f7198212fabff2c5145e7f726ca04ae567fe667b141254c7519df290cf63774e5
faaf3954e2f0089b6c6b9965f15e7f9af09c6fb0 fuzz: Extend psbt fuzz target a bit (MarcoFalke)
Pull request description:
Previously it only merged the psbt with itself, now it tries to merge another.
ACKs for top commit:
practicalswift:
Tested ACK faaf3954e2f0089b6c6b9965f15e7f9af09c6fb0
Tree-SHA512: e1b1d31a47d35e1767285bc2fda176c79cb0550d6d383fe467104272e61e1c83f6cbc0c7d6bbc0c3027729eec13ae1f289f8950117ee91e0fb3703e66d5e6918
fa6183d7761eef8bb815aa888f0396e92e275f05 test: Remove option to make TestChain100Setup non-deterministic (MarcoFalke)
fa732bccb3cf9bc2cdc444975286df0e799917a2 test: Use compressed keys in TestChain100Setup (MarcoFalke)
Pull request description:
Seems odd to have an option for non-deterministic tests
when the goal should be for all tests to be deterministic.
ACKs for top commit:
jamesob:
ACK fa6183d776
practicalswift:
cr ACK fa6183d7761eef8bb815aa888f0396e92e275f05: patch looks deterministic!
Tree-SHA512: 6897a9f36e0dfb7d63b25dd6984414b3ee8a62458ad232cb21ed5077184fdb0bc626996e4ac84ef0bdd452b9f17c54aac75a71575b8e723b84cac07c9f9d5611
9044522ef76f880760165d98fab024802ccfc062 Drop JSONRPCRequest constructors after #21366 (Russell Yanofsky)
Pull request description:
This just makes an additional simplification after #21366 replaced
util::Ref with std::any. It was originally suggested
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351 but
delayed for a followup. It would have prevented usage bug
https://github.com/bitcoin/bitcoin/pull/21572.
ACKs for top commit:
promag:
ACK 9044522ef76f880760165d98fab024802ccfc062, fixed conflict in src/wallet/interfaces.cpp.
Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
Seems odd to have an option for non-deterministic tests
when the goal should be for all tests to be deterministic.
Can be reviewed with `--ignore-all-space`.
fa9b74f5ea89624e052934c48391b5076a87ffef Fix assumeutxo crash due to missing base_blockhash (MarcoFalke)
fa8fffebe8ac126f31143619843dd6578a2f4e3c refactor: Prefer clean assert over UB in coinstats (MarcoFalke)
Pull request description:
This fixes an UB (which results in a crash with sanitizers enabled). Can be reproduced by cherry-picking the test without the other code changes. The fix:
* Adds an `Assert` to transform the UB into a clean crash, even when sanitizers are disabled
* Adds an early-fail condition to avoid the crash
ACKs for top commit:
jamesob:
ACK fa9b74f5ea89624e052934c48391b5076a87ffef ([`jamesob/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21582.1.MarcoFalke.fix_assumeutxo_crash_due))
ryanofsky:
Code review ACK fa9b74f5ea89624e052934c48391b5076a87ffef with no code changes since last review, just splitting up combocommit a little.
Tree-SHA512: dd36808a09f49c647543a9eaa6fdb785b3f1109af48ba4cc983153b22a144da9ca61af22034dcfaa0e192a65b1ee7de744f187555079aff55bec0efa0ce87cd4
81747b21719b3fa6b0fdfc3b084c0104d64903f9 test: make sure non-IP peers get discouraged and disconnected (Vasil Dimov)
637bb6da368b87711005b909f451f94909400092 test: also check disconnect in denialofservice_tests/peer_discouragement (Vasil Dimov)
4d6e246fa46f2309e2998b542e4c104d73d29071 test: use pointers in denialofservice_tests/peer_discouragement (Vasil Dimov)
Pull request description:
Split up from #20966, so that it can be backported easier. Merging this ahead of #20966 will also reduce the number of conflicts for that pull.
ACKs for top commit:
jonatack:
ACK 81747b21719b3fa6b0fdfc3b084c0104d64903f9
Tree-SHA512: 8f0e30b95baba7f056920d7fc3b37bd49ee13e69392fe80e2d333c6bb09fd25f4603249301b8795cca26a2f2d15b9f8904798a55cd9c04fd28afb316e95c551c
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.
The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.
If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.
