done to reduce the number of conflicts associated with the unexpected
forward decl, moved it just above WriteAutoBitSet and added comment to
clarify reason.
c09981d8ad fix: drop symlinks in immer subtree (pasta)
Pull request description:
## Issue being fixed or feature implemented
I've been playing around recently with the `github-merge.py` script recently; and while I've been able to create merges with it (see 73c90c8370 and 10ddf62dfb) it complains / refuses to trivially work with symlinks. I haven't been able to figure out exactly why it doesn't want symlinks, but I think it's related to the sha512sum hash that gets created.
I'm not sure if we'll start using `github-merge.py` more or not; but I guess we should "fix" this anyhow in order to learn more how these work. It may be useful to move to merging commits locally and not relying on github being a good actor? 🤷
## What was done?
## How Has This Been Tested?
Ran `github-merge.py` locally and it no longer complains about symlinks
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: be935bb9f5fa562fa7af5b33da239b6eca06bcf2790ee3f16d1b313b8b5ad1c878f4761b855cea5f7776f0241622f86feac91149a74c6d8940ddb8fb1b5700c4
I've been playing around recently with the `github-merge.py` script recently; and while I've been able to create merges with it (see 73c90c8370 and 10ddf62dfb)
it complains / refuses to trivially work with symlinks. I haven't been able to figure out exactly why it doesn't want symlinks, but I think it's related to the sha512sum hash that gets created.
I'm not sure if we'll start using `github-merge.py` more or not; but I guess we should "fix" this anyhow in order to learn more how these work. It may be useful to move to merging commits locally and not relying on github being a good actor? 🤷
a852a919dd Merge bitcoin/bitcoin#25697: depends: expat 2.4.8 & fix building with -flto (fanquake)
587c335c54 Merge bitcoin/bitcoin#22485: doc: BaseIndex sync behavior with empty datadir (MacroFake)
aaa170af05 Merge bitcoin/bitcoin#25605: depends: update urls for dmg tools (fanquake)
7cbf69dd56 Merge bitcoin/bitcoin#24319: refactor: Avoid unsigned integer overflow in core_write (MarcoFalke)
002db515dc Merge bitcoin/bitcoin#24191: refactor: Make MessageBoxFlags enum underlying type unsigned (MarcoFalke)
93027376bf Merge bitcoin/bitcoin#24059: Fix implicit-integer-sign-change in arith_uint256 (MarcoFalke)
7e57600a22 Merge bitcoin/bitcoin#23992: fuzz: Limit fuzzed time to years 2000-2100 (MarcoFalke)
cd13274076 Merge bitcoin/bitcoin#23626: refactor: Fix implicit-signed-integer-truncation in cuckoocache.h (fanquake)
2a4558b4f7 Merge bitcoin/bitcoin#23553: test: Remove sanitizer suppression implicit-signed-integer-truncation:netaddress.cpp (fanquake)
75c877dbba Merge bitcoin/bitcoin#22584: test: Add temporary sanitizer suppression implicit-signed-integer-truncation:netaddress.cpp (MarcoFalke)
c342ce95b8 Merge bitcoin/bitcoin#22146: Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)
0557c32264 Merge bitcoin/bitcoin#22202: test: Add temporary coinstats suppressions (MarcoFalke)
3450fa5fe4 Merge bitcoin/bitcoin#21846: fuzz: Add `-fsanitize=integer` suppression needed for RPC fuzzer (`generateblock`) (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
## How Has This Been Tested?
Building locally; tests not ran
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 42e7fa81ce409917b36fa761b6a48a3869f5efe7287b8f02bb120036d6b7a6ff3463477cce4bcf87957f24c9128abcfbce4dfe0c4a0a3a52f9690527f49294f2
e838a9847580527b8321d65e57b1c53cc2af6bf4 depends: re-enable using -flto when building expat (fanquake)
304452558c7f6f5e32ba13d8f05325790c8a4f5f depends: expat 2.4.8 (fanquake)
Pull request description:
Currently, when building the expat package in depends, using `-flto` (`LTO=1`), the configure check can fail, because it cannot determine the system endianess:
```bash
configure:18718: result: unknown
configure:18733: error: unknown endianness
presetting ac_cv_c_bigendian=no (or yes) will help
```
Fix that by defining `_DEFAULT_SOURCE`, which in turn defines `__USE_MISC` (`features.h`):
```c
#if defined _DEFAULT_SOURCE
# define __USE_MISC1
#endif
```
which exposes additional definitions in `endian.h`:
```c
#include <features.h>
/* Get the definitions of __*_ENDIAN, __BYTE_ORDER, and __FLOAT_WORD_ORDER. */
#include <bits/endian.h>
#ifdef __USE_MISC
# define LITTLE_ENDIAN__LITTLE_ENDIAN
# define BIG_ENDIAN__BIG_ENDIAN
# define PDP_ENDIAN__PDP_ENDIAN
# define BYTE_ORDER__BYTE_ORDER
#endif
```
and gives us a working configure.
You could test building this change with Guix + LTO with [this branch](https://github.com/fanquake/bitcoin/tree/lto_in_guix). Note that that build may fail for other reasons (on x86_64), unrelated to this change.
Some related upstream discussion:
https://bugs.gentoo.org/757681https://forums.gentoo.org/viewtopic-t-1013786.html
ACKs for top commit:
hebasto:
re-ACK e838a9847580527b8321d65e57b1c53cc2af6bf4, only [suggested](https://github.com/bitcoin/bitcoin/pull/25697#discussion_r929735675) changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/25697#pullrequestreview-1050657421).
jarolrod:
code review ACK e838a9847580527b8321d65e57b1c53cc2af6bf4
Tree-SHA512: 9dbf64c9bd1fd995a4d1addc011ffeff83d50df736030012346c97605e63aed4b5bac390a81abe646c1be28ad6fd600f64560dcb26bbc2edf5d513ca3b180bfa
11780f29e7c3f50fb7717fc98950ece9385d314b doc: BaseIndex sync behavior with empty datadir (James O'Beirne)
Pull request description:
Make a note about a potentially confusing behavior with `BaseIndex::m_synced`;
if the user starts bitcoind with an empty datadir and an index enabled,
BaseIndex will consider itself synced (as a degenerate case). This affects
how indices are built during IBD (relying solely on BlockConnected signals vs.
using ThreadSync()).
ACKs for top commit:
mzumsande:
ACK 11780f29e7c3f50fb7717fc98950ece9385d314b
Tree-SHA512: 0b530379e57c62e05d2ddca7bb8e2c936786fa00678f9eaa1bb3742d957c48f141d46f936734b03f6673d964abc7eb72c1769f1784b9d3563d218e96296b7afd
fa6065661a86656a29e89ed1a3529cb7103f5394 refactor: Avoid unsigned integer overflow in core_write (MarcoFalke)
Pull request description:
Also, I find the new code a bit easier to understand.
ACKs for top commit:
shaavan:
Code Review ACK fa6065661a86656a29e89ed1a3529cb7103f5394
Tree-SHA512: cd751e3b4dc97ef525eb8be8d0a49e9629389cb114df18d59a06e05388822af2939078e937f01494e6b317d601743b1a433ba47aa40c4dc602372d1f0fd0dc11
1111d33532516c16fb2e22660ac2745ce56ad6cd refactor: Make MessageBoxFlags enum underlying type unsigned (MarcoFalke)
Pull request description:
All values in the enum are unsigned. Also, flags shouldn't be treated as signed types. So clarify the underlying type and remove a sanitizer suppression.
ACKs for top commit:
hebasto:
ACK 1111d33532516c16fb2e22660ac2745ce56ad6cd, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 48b16c4a0ace1a1e4d351d6eadadbb1bc42aef7fd82e24e3ea50c62f2c04a552ed21027158d09aa97e630c8c7d732cb150c38065333d7c2accbae46593b7ed9f
fa99e108e778b5169b3de2ce557af68f1fe0ac0b Fix implicit-integer-sign-change in arith_uint256 (MarcoFalke)
Pull request description:
This refactor doesn't change behaviour, but clarifies that the numbers being dealt with aren't supposed to be negative. This helps when reading the code and allows to remove a sanitizer suppression for the whole file.
ACKs for top commit:
PastaPastaPasta:
utACK fa99e108e778b5169b3de2ce557af68f1fe0ac0b
shaavan:
ACK fa99e108e778b5169b3de2ce557af68f1fe0ac0b
Tree-SHA512: f227e2fd22021e39f0445ec041f4a299d13477c23cef0fc06c53fb3313cbe550cec329336224a7e8775d9045b8009423052b394e83d42a1e40772085dfcdd471
fa7238300c18938cdf627cacfc58d4b81602417f fuzz: Limit fuzzed time to years 2000-2100 (MarcoFalke)
Pull request description:
It doesn't make sense to fuzz times in the past, as Bitcoin Core will refuse to start in the past.
Fix that and also remove a sanitizer suppression, which would be hit in net_processing in `ProcessMessage`:
```cpp
if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
addr.nTime = nNow - 5 * 24 * 60 * 60; // <-- Here
```
This changes the format of fuzz inputs. Previously a time value was (de)serialized as 40 bytes, now it is 32 bytes.
ACKs for top commit:
mzumsande:
Code Review ACK fa7238300c18938cdf627cacfc58d4b81602417f
Tree-SHA512: ca6e7233beec2d9ef9fd481d8f1331942a4d2c8fe518b857629bebcc53a4f42ae123b994cf5d359384a0a8022098ff5a9c146600bc2593c6d88734e25bc240ad
fa7da227daf8558be14f226c4366583fdc59ba10 refactor: Fix implicit-signed-integer-truncation in cuckoocache.h (MarcoFalke)
Pull request description:
Using a file-wide suppression for this implicit truncation has several issues:
* It is file-wide, thus suppressing any other (newly introduced) issues
* The file doesn't compile with `-Wimplicit-int-conversion`
Fix both issues by making the truncation explicit.
ACKs for top commit:
fanquake:
ACK fa7da227daf8558be14f226c4366583fdc59ba10
Tree-SHA512: bf2076ed94c4e80d0d29ff883080edc7a73144c73d6d3e872ec87966177ee3160f4760fc4c774aaa6fb591f4acee450a24b0f7c82291e0bef96582a6d134046e
fae5fec0fec851568a72724000193b2747c30414 test: Remove sanitizer suppression implicit-signed-integer-truncation:netaddress.cpp (MarcoFalke)
Pull request description:
This reverts commit fa865287e5f35e0a376785834e966dd202d2959e.
This was fixed in commit efd6f904c78769ad2e93c1f1de43014d284e7561.
ACKs for top commit:
vasild:
ACK fae5fec0fec851568a72724000193b2747c30414
Tree-SHA512: 3bebf1babd5c68cbb2506bcab9b8e9ffed8697213cf66190484748741f05c59b847a103be171360f7fd6ddb57dfd86ed34a123f72860b76e533ed46bb53a4852
fa865287e5f35e0a376785834e966dd202d2959e test: Add temporary sanitizer suppression implicit-signed-integer-truncation:netaddress.cpp (MarcoFalke)
Pull request description:
This is required to unbreak the fuzzers while a fix is being worked on.
https://cirrus-ci.com/task/4787303177519104?logs=ci#L3020
```
netaddress.cpp:1190:18: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint8_t' (aka 'unsigned char') changed the value to 255 (8-bit, unsigned)
ACKs for top commit:
practicalswift:
cr ACK fa865287e5f35e0a376785834e966dd202d2959e
tryphe:
untested ACK fa865287e5f35e0a376785834e966dd202d2959e
lsilva01:
ACK fa865287e5
Tree-SHA512: 4a54ec68c014c7a4c9ab268c3a04321db5eb9b2857646b41406d8d4908a3d349848b4549e80aea6afd9a0c3639522a48fe578527139519b12439eae9f0c4c46c
fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4 Reject invalid coin height and output index when loading assumeutxo (MarcoFalke)
Pull request description:
It should be impossible to have a coin at a height higher than the height of the snapshot block, so reject those early to avoid integer wraparounds and hash collisions later on.
Same for the outpoint index.
Both issues were found by fuzzing:
* The height issue by OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793
* The outpoint issue by my fuzz server: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34793#c2
ACKs for top commit:
practicalswift:
cr ACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4: patch looks correct
jamesob:
crACK fa9ebedec3
theStack:
Code review ACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4
benthecarman:
crACK fa9ebedec3f982bb5bb459ea33d74c94d9b5cec4
Tree-SHA512: dae7caee4b3862b23ebdf2acb7edec4baf75b0dbf1409b370b1a73aa6b632b317ebfac596dcbaf4edfb1301b513f45465ea75328962460f35e2af0d7e547c9ac
faca40ec68a25180f90a5b9ef017f931354d5bc6 test: Add temporary coinstats suppressions (MarcoFalke)
Pull request description:
Needed for my fuzzer to continue to run
ACKs for top commit:
practicalswift:
cr ACK faca40ec68a25180f90a5b9ef017f931354d5bc6: suppression looks necessary (temporarily)
Tree-SHA512: 5bdff9a24a60546cfe31e775fa2aa5e238aefda2ed2604bef18c82b1b80c51ca3cbe058d6c7988fa75305258b70076036a3e430b9b7de13a111309fa7a66745b
575792e6ffe23c8236a1f8431f6be445e448809b fuzz: Add -fsanitize=integer suppression needed for RPC fuzzer (practicalswift)
Pull request description:
Add `-fsanitize=integer` suppression needed for RPC fuzzer (`generateblock`).
Context: https://github.com/bitcoin-core/qa-assets/pull/59/checks?check_run_id=2494624259
```
miner.cpp:130:21: runtime error: implicit conversion from type 'int64_t' (aka 'long') of value 244763573890 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4245405314 (32-bit, unsigned)
#0 0x56143974eaf3 in BlockAssembler::CreateNewBlock(CScript const&) miner.cpp:130:21
#1 0x56143993690d in generateblock()::$_4::operator()(RPCHelpMan const&, JSONRPCRequest const&) const rpc/mining.cpp:370:127
```
ACKs for top commit:
practicalswift:
> review ACK [575792e](575792e6ff), but this function shouldn't be called by the rpc fuzzer, at least not without sanitizing num_blocks
MarcoFalke:
review ACK 575792e6ffe23c8236a1f8431f6be445e448809b
Tree-SHA512: c2133d1064bf17df0e7749ef4a0f7664b5c8082040491a1035d39f0c6e5d96997b347ef2354411e285c7f1f973e34515f1c3c88eb3de60fab64ca4d2adf6dd74
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
d033cd55be partial bitcoin#26341: add BIP158 false-positive element check in rpc_scanblocks.py (Kittywhiskers Van Gogh)
eab94ac07b merge bitcoin#23859: Add missing suppressions for crypto_diff_fuzz_chacha20.cpp (Kittywhiskers Van Gogh)
0e8d4a1a95 partial bitcoin#21798: Create a block template in tx_pool targets (Kittywhiskers Van Gogh)
ad71db2dcc merge bitcoin#21604: Document why no symbol names can be used for suppressions (Kittywhiskers Van Gogh)
c116d8405a merge bitcoin#21599: Replace file level integer overflow suppression with function level suppression (Kittywhiskers Van Gogh)
8a0dc8cfa1 merge bitcoin#21586: Add missing suppression for signed-integer-overflow:txmempool.cpp (Kittywhiskers Van Gogh)
53cf0d5cea merge bitcoin#21000: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/5902
* [bitcoin#21586](https://github.com/bitcoin/bitcoin/pull/21586) was listed as DNM in the backports Google Sheet as listed below as it is `reverted as #23059`
* [bitcoin#23059](https://github.com/bitcoin/bitcoin/pull/23059) actually reverts [bitcoin#22925](https://github.com/bitcoin/bitcoin/pull/22925) (which deals with `addrman.cpp`), not [bitcoin#21586](https://github.com/bitcoin/bitcoin/pull/21586) (which deals with `txmempool.cpp`).
## Breaking Changes
None, changes are limited to fuzzing, functional tests and undefined behavior exclusions
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 7b6c3fcb462edc9bbcc12a0c4eab052d3ef3a6048281b162dd5650f311f6e1bae7a958adf1c03aa0dea09c56c1ddbc99d98a44023fd81f83b2325fd79dc32e80
098748ac6a Merge bitcoin/bitcoin#25589: test: speedup wallet_coinbase_category.py (MacroFake)
8cc7f38017 Merge bitcoin/bitcoin#25568: Remove my key from trusted-keys (MacroFake)
d1018ff55a Merge bitcoin/bitcoin#25506: Rephrase error message for invalid value of `-peertimeout` (MacroFake)
64a6f74de1 Merge bitcoin/bitcoin#25457: Use more specific path when including `memenv.h` header (laanwj)
581dba9914 Merge bitcoin/bitcoin#25451: test: `-whitebind` and `-bind` with `-listen=0` should throw an error (MacroFake)
6a8b3f2b40 Merge bitcoin/bitcoin#25440: log: Use consistent wording in random.cpp log (MacroFake)
6af409da1a Merge bitcoin/bitcoin#25425: build: Fix `::_wsystem` check (laanwj)
fa14df62bf Merge bitcoin/bitcoin#25370: test: check for `getblocktxn` request with out-of-bounds tx index (MacroFake)
da4fd2eaf2 Merge bitcoin/bitcoin#25367: [contrib] message-capture-parser: fix out of bounds error for empty vectors (MacroFake)
Pull request description:
## Issue being fixed or feature implemented
batch of trivial backports; small batch as a lot of them ended up causing test failures
## What was done?
## How Has This Been Tested?
Building locally; tests not ran
## Breaking Changes
Didn't see any yet; didn't review much yet
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: e35c74c3f93556e42803da659ded780a8c2ba60a7ae322b8e15590189ddcef47dbccbd42b195c4a09a3ba149dc53c129e0c761cd299d66a8e40b7b4bc5fd4e87
76a84c0a6cac3df711b1ed907a46c905cb85c485 test: speedup wallet_coinbase_category.py (furszy)
Pull request description:
No need to create a chain (200 extra blocks), nor use the cache, for it.
Top commit has no ACKs.
Tree-SHA512: beec64ba6c580d475e19700371ae155f4f3d74325879802da83d02f4153a752d5829b8a4ed77e0c893e79cbc26f66389eed90ac2e8b03a59f6c630ee9333355c
ceec6808d331fa082407a734cd5f3c2f1c7d11b3 test: `-whitebind` and `-bind` with `-listen=0` should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
b9122e95f0/src/init.cpp (L872-L875)
ACKs for top commit:
laanwj:
Code review ACK ceec6808d331fa082407a734cd5f3c2f1c7d11b3
Tree-SHA512: 03068abe7199b1235f029871ab87a3dd4943738c592ad62d82cdcd3e0201e627624960bd3ea1fc6fc1e7da4b8e215ba3393d1cb8130e1108049f764e51dc75c0
c01ae8f5ead6837162a2772ce792a83f66757ee4 Use consistent wording in log (Igor Bubelov)
Pull request description:
It's a trivial change, but it bothers me a bit that two log lines in a row aren't grammatically identical while following exactly the same pattern. I've read `contributing.md` and I'm aware that changes like this are usually being ignored and dropped, but I decided to leave it here anyway in case someone feels the same way about inconsistent log messages or grammar =)
ACKs for top commit:
laanwj:
Code review ACK c01ae8f5ead6837162a2772ce792a83f66757ee4
Tree-SHA512: d5b3849b3a6e3de7ea9b468c05f17cacd1dbd1aca2f3401b5138383dc8d385cea9e221db558ab472c1c4c7f6921d57dcc7af89a54776c5765fa00e429694b4e7
5a8c321444c10c7c89c4222c0b47c2d83a1a9ea4 test: check for `getblocktxn` request with out-of-bounds tx index (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the `getblocktxn` message handler, in the case that any of the contained indices is out-of-bounds:
a05876619a/src/net_processing.cpp (L2180-L2183)
ACKs for top commit:
dunxen:
ACK 5a8c321
Tree-SHA512: 2743c2c6d8aed57b22f825aefd60ba3e670321b60625a42ea7248e7b0fc41c73e9a5945153567c02824ba3b5f0fce7f4125bffc974973fc608b6ffbe49e14b65
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
42bbbba7c83d1e2baad18b4c6f05bad1358eb117 message-capture-parser: fix out of bounds error for empty vectors (Sebastian Falbesoner)
Pull request description:
The script [message-capture-parser.py](https://github.com/bitcoin/bitcoin/blob/master/contrib/message-capture/message-capture-parser.py) currently throws an "out of bounds" error if a message containing an empty integer vector element is tried to converted to JSON (e.g. by the BIP157 message `cfcheckpt` with empty `FilterHeaders` vector):
```
Traceback (most recent call last):
File "/home/honey/bitcoin/./contrib/message-capture/message-capture-parser.py", line 217, in <module>
main()
File "/home/honey/bitcoin/./contrib/message-capture/message-capture-parser.py", line 202, in main
process_file(str(capture), messages, "recv" in capture.stem, progress_bar)
File "/home/honey/bitcoin/./contrib/message-capture/message-capture-parser.py", line 162, in process_file
msg_dict["body"] = to_jsonable(msg)
File "/home/honey/bitcoin/./contrib/message-capture/message-capture-parser.py", line 85, in to_jsonable
elif slot in HASH_INT_VECTORS and isinstance(val[0], int):
IndexError: list index out of range
```
Fix this by using the `all(...)` predicate rather to access the first element `val[0]` (which in the error case doesn't exist).
ACKs for top commit:
laanwj:
Code review ACK 42bbbba7c83d1e2baad18b4c6f05bad1358eb117
Tree-SHA512: 139ec6b90304a69f26ec731e6f12b216fa10e554f777505b61adfa1e569f6861a4a849159dd1eae7a1aa0427e8598af226b6f0c4015020dcac8ab109fbc35dba
fb272dd8ff refactor: use NodeContext members instead of globals in interface logic (Kittywhiskers Van Gogh)
aba57cecb4 qt: add interface for CGovernanceObject querying (Kittywhiskers Van Gogh)
8d73cec438 refactor: reduce globals use in RPC and bench, use LLMQContext members (Kittywhiskers Van Gogh)
e8270ca91b refactor: use NodeContext members instead of globals in RPC logic (Kittywhiskers Van Gogh)
2efebcac81 refactor: use aliases of globals in unit tests (Kittywhiskers Van Gogh)
89f41fa826 refactor: dereference CDeterministicMNManager instance before use (Kittywhiskers Van Gogh)
d3dfdf353c refactor: use aliases of globals in initialization logic (Kittywhiskers Van Gogh)
bb7fe582f8 refactor: rename creditPoolManager to cpoolman in NodeContext, fix order (Kittywhiskers Van Gogh)
6f08e10540 refactor: remove extraneous creditPoolManager initialization (Kittywhiskers Van Gogh)
d136c407d7 refactor: add aliases for Dash-specific global pointers in NodeContext (Kittywhiskers Van Gogh)
Pull request description:
## Additional Notes
* In some limited circumstances, we need to pass a reference to a smart pointer _before_ it is initialized. This is permissible if we know for sure that the pointer will be dereferenced _after_ it is initialized (usually, by `assert`'ing `!= nullptr`). This requires us to pass the const ref of the smart pointer object, to ensure that when dereferenced, we get access to the updated value that is set later on the initialization sequence.
This means we cannot use the bare pointer alias as an argument as it would remain `nullptr` and will not be updated.
Currently known examples are:
* `CChainState` through `ChainstateManager::InitializeChainstate`
* `llmq::CChainLocksHandler` (as `llmq::chainLocksHandler`)
* `llmq::CInstantSendManager` (as `llmq::quorumInstantSendManager`)
* `llmq::CQuorumBlockProcessor` (as `llmq::quorumBlockProcessor`)
* `CDSNotificationInterface`
* `CDeterministicMNManager` (as `deterministicMNManager`)
* `CJContext` (as `NodeContext::cj_ctx`)
* `LLMQContext` (as `NodeContext::llmq_ctx`)
* We can verify the absence of globals in GUI, interface, RPC and test logic with the commands below (the above caveat applies)
```bash
$ function make_filter() { echo "$1->|\*$1|::$1"; }
$ PROHIBITED_TERMS="$(make_filter creditPoolManager)|$(make_filter deterministicMNManager)|$(make_filter dstxManager)|$(make_filter governance)|$(make_filter mmetaman)|$(make_filter netfulfilledman)|$(make_filter sporkManager)";
$ PROHIBITED_LLMQ_TERMS="$(make_filter quorumBlockProcessor)|$(make_filter quorumManager)|$(make_filter chainLocksHandler)|$(make_filter quorumInstantSendManager)";
$ grep -E "$PROHIBITED_TERMS|$PROHIBITED_LLMQ_TERMS" src/bench/*.{cpp,h} src/node/interfaces.cpp src/qt/*.{cpp,h} src/qt/test/*.{cpp,h} src/rpc/*.{cpp,h} src/test/*.{cpp,h} src/wallet/rpc*.{cpp,h};
src/test/validation_chainstate_tests.cpp: CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool, *m_node.mnhf_manager, *m_node.evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor));
src/test/validation_chainstatemanager_tests.cpp: CChainState& c1 = WITH_LOCK(::cs_main, return manager.InitializeChainstate(&mempool, *m_node.mnhf_manager, evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor));
src/test/validation_chainstatemanager_tests.cpp: &mempool, *m_node.mnhf_manager, evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor,
src/test/validation_chainstatemanager_tests.cpp: CChainState& c1 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool, *m_node.mnhf_manager, evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor));
src/test/validation_chainstatemanager_tests.cpp: CChainState& c2 = WITH_LOCK(cs_main, return manager.InitializeChainstate(&mempool, *m_node.mnhf_manager, evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor, GetRandHash()));
src/test/validation_flush_tests.cpp: CChainState chainstate(&mempool, blockman, *m_node.mnhf_manager, *m_node.evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor);
```
* We can also check the usage of globals in (test) initialization logic (it will also include all the actual (de)initialization logic and any usage of the bare pointer alias if the name of the global and the alias are the same, as is the case with `netfulfilledman`)
```bash
$ grep -E "$PROHIBITED_TERMS|$PROHIBITED_LLMQ_TERMS" src/init.cpp src/test/util/setup_common.cpp;
src/init.cpp: ::netfulfilledman.reset();
src/init.cpp: ::mmetaman.reset();
src/init.cpp: ::dstxManager.reset();
src/init.cpp: ::sporkManager.reset();
src/init.cpp: ::governance.reset();
src/init.cpp: assert(!::governance);
src/init.cpp: ::governance = std::make_unique<CGovernanceManager>();
src/init.cpp: node.govman = ::governance.get();
src/init.cpp: assert(!::sporkManager);
src/init.cpp: ::sporkManager = std::make_unique<CSporkManager>();
src/init.cpp: node.sporkman = ::sporkManager.get();
src/init.cpp: *node.connman, *node.mn_sync, ::deterministicMNManager, *node.govman, node.llmq_ctx, node.cj_ctx
src/init.cpp: chainman.InitializeChainstate(Assert(node.mempool.get()), *node.mnhf_manager, *node.evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor);
src/init.cpp: assert(!::dstxManager);
src/init.cpp: ::dstxManager = std::make_unique<CDSTXManager>();
src/init.cpp: node.dstxman = ::dstxManager.get();
src/init.cpp: assert(!::mmetaman);
src/init.cpp: ::mmetaman = std::make_unique<CMasternodeMetaMan>(fLoadCacheFiles);
src/init.cpp: node.mn_metaman = ::mmetaman.get();
src/init.cpp: assert(!::netfulfilledman);
src/init.cpp: ::netfulfilledman = std::make_unique<CNetFulfilledRequestManager>(fLoadCacheFiles);
src/init.cpp: node.netfulfilledman = ::netfulfilledman.get();
src/init.cpp: if (!node.netfulfilledman->IsValid()) {
src/test/util/setup_common.cpp: ::deterministicMNManager = std::make_unique<CDeterministicMNManager>(chainstate, *node.connman, *node.evodb);
src/test/util/setup_common.cpp: node.dmnman = ::deterministicMNManager.get();
src/test/util/setup_common.cpp: ::deterministicMNManager.reset();
src/test/util/setup_common.cpp: ::sporkManager = std::make_unique<CSporkManager>();
src/test/util/setup_common.cpp: m_node.sporkman = ::sporkManager.get();
src/test/util/setup_common.cpp: ::governance = std::make_unique<CGovernanceManager>();
src/test/util/setup_common.cpp: m_node.govman = ::governance.get();
src/test/util/setup_common.cpp: ::dstxManager = std::make_unique<CDSTXManager>();
src/test/util/setup_common.cpp: m_node.dstxman = ::dstxManager.get();
src/test/util/setup_common.cpp: ::mmetaman = std::make_unique<CMasternodeMetaMan>(/* load_cache */ false);
src/test/util/setup_common.cpp: m_node.mn_metaman = ::mmetaman.get();
src/test/util/setup_common.cpp: ::netfulfilledman = std::make_unique<CNetFulfilledRequestManager>(/* load_cache */ false);
src/test/util/setup_common.cpp: m_node.netfulfilledman = ::netfulfilledman.get();
src/test/util/setup_common.cpp: ::netfulfilledman.reset();
src/test/util/setup_common.cpp: ::mmetaman.reset();
src/test/util/setup_common.cpp: ::dstxManager.reset();
src/test/util/setup_common.cpp: ::governance.reset();
src/test/util/setup_common.cpp: ::sporkManager.reset();
src/test/util/setup_common.cpp: m_node.chainman->InitializeChainstate(m_node.mempool.get(), *m_node.mnhf_manager, *m_node.evodb, llmq::chainLocksHandler, llmq::quorumInstantSendManager, llmq::quorumBlockProcessor);
```
## Breaking Changes
None, changes are limited to refactoring and do not logically change behaviour.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: f6d1a668728525e7cab7af200858a6ab6e20a24786d691630530a9019e8d2b4f474ed4358f40b76266b938dfe96420e0c64ee66a1a0986a3d3f254670162bb1c
The GetAbsoluteYesCount -> Get{Yes,No}Count -> CountMatchingVotes call
chain eventually results in needing to call GetListAtChainTip from the
CDeterministicMNManager instance.
Currently it's being accessed as a global but future deglobalization
will move fetching the CDeterministicMNList outside CGovernanceObject,
requiring an instance of CDeterministicMNManager.
In preparation of that, these changes are being made in advance.
We must pass ::deterministicMNManager to CDSNotificationInterface as
we are passing a const ref of the smart pointer object, because it is
init'ed further down the line.
The alias in NodeContext is a raw pointer that is assigned after init
and is unsuitable for this purpose.
