fadddd13eef4428f5fa7237583d4be41a9335cd9 test: Add missing syncwithvalidationinterfacequeue (MarcoFalke)
faa211fc6e3d4984b8edff1d762dd4cba205d982 test: Misc cleanup (MarcoFalke)
fa1668bf5084a190b26b022b9e625a7be3defa6e test: Run pep-8 (MarcoFalke)
facd97ae0f0d816107aa3bc9de321244200636a0 scripted-diff: Renames (MarcoFalke)
Pull request description:
The index on the block filters is running in the background on the validation interface. To avoid intermittent test failures, it needs to be synced.
Also other cleanups.
ACKs for top commit:
lsilva01:
Tested ACK fadddd13ee on Ubuntu 20.04
Tree-SHA512: d858405db426a2f9d5620059dd88bcead4e3fba3ccc6bd8023f624b768fbcfa2203246fb0b2db620490321730d990f0e78063b21a26988c969cb126d4bd697bd
39a9ec579f023ab262a1abd1f0c869be5b1f3f4d Unconditionally check for fRelay field in test framework (Troy Giorshev)
Pull request description:
picking up #20411 (rebased onto master)
There is a discrepancy in the implementation of our p2p protocol between
bitcoind and the testing framework. The fRelay field is an optional
field at the end of a version message as of protocol version 70001.
However, when deserializing a message in bitcoind, we don't check the
version to see if it should have an fRelay field or not. Instead, we
unconditionally attempt to deserialize into the field.
This commit brings the testing framework in line with the implementation
in core.
This matters for a version message with the following fields:
Version = 60000
fRelay = 1
Bitcoind would deserialize this into a version message with
Version=60000 and fRelay=1, whereas (before this commit) our testing
framework would deserialize this into a version message with
Version=60000 and fRelay=0.
ACKs for top commit:
jnewbery:
utACK 39a9ec579f023ab262a1abd1f0c869be5b1f3f4d
Tree-SHA512: 13a23f1180b7121ba41cb85baa38094b41f4607a7c88b3384775177cb116e76faf5514760624f98a4e8a830767407c46753a7e0285158c33e0c6ce395de8f15c
fa72fce7c948185752a01002000ea511809146ed test: Use ConnmanTestMsg from test lib in denialofservice_tests (MarcoFalke)
Pull request description:
This allows to remove code.
Also, required for https://github.com/bitcoin/bitcoin/pull/18470
ACKs for top commit:
mjdietzx:
crACK fa72fce7c948185752a01002000ea511809146ed 👍👍
fanquake:
ACK fa72fce7c948185752a01002000ea511809146ed
Tree-SHA512: 12aa68cde697c0f7c25d60bb0c02783e5462eb3ba39947b0d94a7798bc278e7d5f092f3ab2a3d0547947c3502cde7c4a599419055a57f78ef1f70f9f637e14c7
6c3fcd5591eb9947f35483014ecb0d8ab217b780 test: remove BasicTestingSetup from util_threadnames unit tests (fanquake)
b53d3c1b1fd739c314b0b34f361fcd992092fc29 test: remove BasicTestingSetup from uint256 unit tests (fanquake)
c0497a49281e68b57e2a1e6c48c950b2edc80821 test: remove BasicTestingSetup from torcontrol unit tests (fanquake)
ef8bb0473be62c07f96eb269b927dcec86c1e862 test: remove BasicTestingSetup from sync unit tests (fanquake)
1aee83421fe2128757b48f6317a3e7fed784adb6 test: remove BasicTestingSetup from reverse_lock unit tests (fanquake)
57ba949ef585f8124914c43ea9a53afee201b998 test: remove BasicTestingSetup from policy_fee unit tests (fanquake)
3974c962b61a1e18f8177ffa30791ad9ad2ba6e4 test: remove BasicTestingSetup from merkleblock tests (fanquake)
cd5bc4b4708b28cabcfabbcd7f5ba1155f5b1517 test: remove BasicTestingSetup from hash unit tests (fanquake)
39cec22935302418963cc2e7db4ad2fa9656849d test: remove BasicTestingSetup from compilerbug unit tests (fanquake)
6d3b78c0e2f427d3a7431885cc175464a527a12a test: remove BasicTestingSetup from bswap unit tests (fanquake)
a13dc24831e4a2d8e16a41d8c95cdaa8afdec783 test: remove BasicTestingSetup from bech32 unit tests (fanquake)
f4dcbe4498e55d2ed818b35cd15652fd427b7a7b test: remove BasicTestingSetup from base64 unit tests (fanquake)
fd144f64265a4752fe36391c51bb6b8ccdff838f test: remove BasicTestingSetup from base32 unit tests (fanquake)
4c389ba04b36cc2916d49435e07155144882a637 test: remove BasicTestingSetup from arith_uint256 unit tests (fanquake)
05590651a0b9ebc5f5fdbdcbbc1efe4bf64888d0 test: remove BasicTestingSetup from amount unit tests (fanquake)
883a5c7d021fe29539d417796a5b07e265f1c696 test: remove BasicTestingSetup from allocator unit tests (fanquake)
Pull request description:
* Less setup/overhead for tests that don't need it. Some naive bench-marking would suggest that a full `test_bitcoin` run is a few % faster after this change.
* Tests which don't need the BasicTestingSetup can't accidentally end up depending on it somehow.
* Already the case in at least the scheduler and block_filter tests.
This adds missing includes, but more significant is the removal of `setup_common.h` from tests where it isn't needed. This saves recompiling those tests when changes are made in the header.
ACKs for top commit:
practicalswift:
cr ACK 6c3fcd5591eb9947f35483014ecb0d8ab217b780: patch looks correct
laanwj:
ACK 6c3fcd5591eb9947f35483014ecb0d8ab217b780
Tree-SHA512: 69b891e2b4740402d62b86a4fc98c329a432d125971342a6f97334e166b3537ed3d4cdbb2531fa05c1feae32339c9fcb2dceda9afeeaed4edc70e8caa0962161
fa576b4532814b4bca1936d170cabd01fbc51960 Move MakeNoLogFileContext to common libtest_util, and use it in bench (MarcoFalke)
Pull request description:
To avoid verbose code duplication, which may lead to accidental mishaps https://github.com/bitcoin/bitcoin/pull/20998/files#r563624041.
Also fix a nit I found in https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
ACKs for top commit:
dongcarl:
Light Code-Review ACK fa576b4532814b4bca1936d170cabd01fbc51960
fanquake:
ACK fa576b4532814b4bca1936d170cabd01fbc51960
Tree-SHA512: d39ac9c0957813ebb20ed13bd25a8ef8469377ce2651245638bd761c796feac2a17e30dd16f1e5c8db57737fb918c81d56a3d784c33258275e426a1b11e69eb2
86c495223f048e5ca2cf0d8730af7db3b76f7aba net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack)
6609eb8cb50fe92c7317b5db9e72d4333b3aab1b net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack)
993d1ecd191a7d9161082d4026f020cbf00835bb test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack)
Pull request description:
The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in #20197:
- asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary
- fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor
- drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status
- adds a public getter `IsInboundOnion()` that also allows unit testing it
- adds unit test coverage
ACKs for top commit:
sipa:
utACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
LarryRuane:
ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
vasild:
ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba
MarcoFalke:
review ACK 86c495223f048e5ca2cf0d8730af7db3b76f7aba 🐍
Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204
fada8b019af104a0df7659ede2618f594bb3e78a test: Add missing assignment in mempool_resurrect.py (MarcoFalke)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: f438d85cd14a91eabfc380d9ee120cc7a7f9103cf0cd1cf565f675f386f82d966901c0ad3f60b8c462642fbf0a3791dbbd774f9b07668d22b58eb575c8d702c1
11a32722f09f1d81f34bd09b26248ba99f2e7f07 test: run mempool_resurrect.py even with wallet disabled (Michael Dietz)
Pull request description:
Another functional test rewritten as proposed in #20078
**Request for help:**
`node.gettransaction(txid)` fails for transactions sent with `wallet.send_self_transfer`. Even though the `txid`s look correct, are added to the mempool correctly, and removed from the mempool when a block is mined - all as expected.
However, `node.gettransaction(txid)` throws the error:
```sh
Traceback (most recent call last):
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
self.run_test()
File "/Users/michaeldietz/Documents/bitcoin/test/functional/mempool_resurrect.py", line 43, in run_test
assert_equal(len(list(filter(lambda txid: node.gettransaction(txid)["confirmations"] > 0, spends_ids))), len(spends_ids))
File "/Users/michaeldietz/Documents/bitcoin/test/functional/mempool_resurrect.py", line 43, in <lambda>
assert_equal(len(list(filter(lambda txid: node.gettransaction(txid)["confirmations"] > 0, spends_ids))), len(spends_ids))
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "/Users/michaeldietz/Documents/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Invalid or non-wallet transaction id (-5)
```
Anyone know what's going wrong / can point me in the right direction if I'm making a mistake, or `MiniWallet` needs to be improved for this to work correctly?
ACKs for top commit:
MarcoFalke:
ACK 11a32722f09f1d81f34bd09b26248ba99f2e7f07
Tree-SHA512: 13d83a13ec23920db716e99b68670e61329d1cc73b12063d85bc1679ee6425a9951da4d2e392ca1f27760be7be049ccdc6f504e192ed5cd24ed0ba003b66fab3
6b56c1f4d0d5857d9d61a81dc96db1b603c368b5 test: remove last_{block,header}_equals() in p2p_fingerprint.py (Sebastian Falbesoner)
136d96b71f94bde2c7471ed852d447ec008e3a30 test: use wait_for_{block,header} helpers in p2p_fingerprint.py (Sebastian Falbesoner)
Pull request description:
This small PR takes use of the message receiving helper functions `wait_for_block()` and `wait_for_header()` (from module `test_framework.p2p`) in the test `p2p_fingerprint.py`. It also simplifies the checks for very old stale blocks/headers requests by getting rid of the functions `last_block_equals()` and `last_header_equals()` and rather only testing that not any blocks/headers message is received at all. Unneeded sending of requests are also removed and calls to time.sleep(...) substituted by ping syncs.
ACKs for top commit:
guggero:
ACK 6b56c1f4
Tree-SHA512: 9114db70f3804adad4ab658236762d4fa73fef91158c5756dd1af2d24196ea740451b0028667e0c4047f1f89fe1355031921d3dfb973acc1370052a4bc12c2ab
a392be6cf0 fix: actually run rpc_fundrawtransaction with and without HD feature (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Due to double initialization of `extra_args` rpc_fundrawtransaction always test with enabled or disabled HD wallets (which is enabled by default).
## What was done?
Fixes double initialization and inversion of condition due to hd wallets enabled by default since #5807
## How Has This Been Tested?
Run functional tests and watch `ps aux` during running to ensure that `-usehd` is set properly.
## Breaking Changes
N/A
## Checklist:
- [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
Top commit has no ACKs.
Tree-SHA512: 6e595b4073c50f0355d9f64b966b1a0613a7bb8dae3406523fae737e54725ffc652aa9db30daf46c9640d98f44883780bd075074d56b7e8baeeb0a1d2d007c20
30331a2b61 chore: bump protocol version to 70231 (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
To be merged closer to 20.1 release
## What was done?
## How Has This Been Tested?
## Breaking Changes
n/a
## 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: 63f559df96b3ed1d1cd4b7650d07795fa3b212cb81937f57795b0b46d40f6fc1598b1a8c2f981348653f7b6eb514fa98a7ccd48ffbd691b279114dc0927de1e0
97a331c523 chore: update chainparams for testnet (Konstantin Akimov)
ca0c04d769 docs: update release process for generating seeds: new PR as a reference (Konstantin Akimov)
151b56eacd fix: uninitialized variable onions in makeseeds script (Konstantin Akimov)
b8395aa4e6 chore: update seeds for v20.1 (Konstantin Akimov)
89f3a24517 chore: update chainparams for v20.1 release (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
Part of release process to update seeds and chainparams: https://github.com/dashpay/dash/blob/develop/doc/release-process.md
## Breaking Changes
N/A
## Checklist:
- [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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
Top commit has no ACKs.
Tree-SHA512: 9ba600bbc5aefe2b0ed031a7f1c39d2b105f90b6ecb843c3ab27f0668caaa381dc54cb794f74d6069a72d4d9a3f73dcca782422a98b606b192e65b51fd39b35d
```
$ ./makeseeds.py protx.txt > nodes_main.txt
Traceback (most recent call last):
File "DASH/contrib/seeds/./makeseeds.py", line 183, in <module>
main()
File "DASH/contrib/seeds/./makeseeds.py", line 167, in main
for onion in onions:
^^^^^^
UnboundLocalError: cannot access local variable 'onions' where it is not associated with a value
```
42decd3c68 fix: fallback to a commit hash in `codesign.sh` (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
We don't use tags in https://github.com/dashpay/dash-detached-sigs repo, we use branches so `git describe` fails...
## What was done?
Add `--always` option to fallback to commit hash. Could do
```
git_head_version() {
git -C "$1" rev-parse --short=12 HEAD`
}
```
instead but using `describe` allows us to start using tags one day with no additional patches.
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [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: 71c89641d1e4d81612c7d19d0724627f11b4a986d950483d6bf93d3071b5b0f1de6fd6359eea27a247dc403199d5221aebb2827e89f0bf67ba69a03b1570c668
4c53a434d8 Merge bitcoin/bitcoin#26131: log: log RPC port on startup (MacroFake)
185cf8231e Merge bitcoin/bitcoin#26130: Bugfix: Wallet: Lock cs_wallet for SignMessage (MacroFake)
dc4e834d4a Merge bitcoin/bitcoin#25918: build: prune event2 compat headers (fanquake)
023eb917a8 Merge bitcoin/bitcoin#26090: fs: fully initialize `_OVERLAPPED` for win32 (fanquake)
8de9065a24 Merge bitcoin-core/gui#664: Prevent wrong handling of `%2` token by Transifex (Hennadii Stepanov)
252eae1395 Merge bitcoin/bitcoin#26054: test: verify best blockhash after invalidating an unknown block (MacroFake)
6731b10288 Merge bitcoin/bitcoin#26007: [contrib] message-capture-parser: fix AssertionError on parsing `headers` message (MacroFake)
fa7f8f2d60 Merge bitcoin/bitcoin#26002: build: sync ax_boost_base from upstream (fanquake)
3e24202f50 Merge bitcoin/bitcoin#26038: test: invalidating an unknown block throws an error (MacroFake)
Pull request description:
## Issue being fixed or feature implemented
Trivial backport batch
## What was done?
## How Has This Been Tested?
Ran tests locally; haven't properly reviewed
## 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: b6431432d7a4d3829f1212342cd1c5852baad2e6d42d37d80f40382ed12a05ebd1f580703ae4cada18d2ed84bf80269dd178a7c7e9ef6d43c26229c49d0eebeb
9d14f27bddab351fe98a2ae197bd4cf8a092c4f3 log: log RPC port on startup (James O'Beirne)
Pull request description:
I just spent a few hours trying to figure out why "18444" wasn't getting me to regtest's RPC server. I'm not the sharpest tool in the shed, but I was maybe understandably confused because "Bound to 127.0.0.1:18445" appears in the logs, which I assumed was the P2P port.
This change logs the RPC listening address by default on startup, which seems like a basic piece of information that shouldn't be buried under `-debug`.
ACKs for top commit:
dergoegge:
ACK 9d14f27bddab351fe98a2ae197bd4cf8a092c4f3
jarolrod:
ACK 9d14f27bddab351fe98a2ae197bd4cf8a092c4f3
aureleoules:
ACK 9d14f27bddab351fe98a2ae197bd4cf8a092c4f3
Tree-SHA512: 5c86f018c0b8d6264abf878c921afe53033b23ab4cf289276bb1ed28fdf591c9d8871a4baa4098c363cb2aa9a637d2e4e18e56b14dfc7d767ee40757d7ff2e7c
a60d9eb9e6b6a272a3fca8981d89a55955dced55 Bugfix: Wallet: Lock cs_wallet for SignMessage (Luke Dashjr)
Pull request description:
cs_desc_main is typically locked within scope of a cs_wallet lock, but:
CWallet::IsLocked locks cs_wallet
...called from DescriptorScriptPubKeyMan::GetKeys
...called from DescriptorScriptPubKeyMan::GetSigningProvider which locks cs_desc_main first, but has no access to cs_wallet ...called from DescriptorScriptPubKeyMan::SignMessage ...called from CWallet::SignMessage which can access and lock cs_wallet
Resolve the out of order locks by grabbing cs_wallet in CWallet::SignMessage first
-------------
Note this is currently only an issue for the GUI (which lacks sufficient testing apparently), but can be reproduced by #26082 (CI fails as a result)
ACKs for top commit:
achow101:
ACK a60d9eb9e6b6a272a3fca8981d89a55955dced55
w0xlt:
ACK a60d9eb9e6
Tree-SHA512: 60f6959b0ceaf4d9339ba1a47154734034b637c41b1f9e26748a2dbbc3a2a95fc3696019103c55ae70c91d910ba8f3d7f4e27d263030eb60b689f290c4d82ea9
02c9e564687af6ae2b0b6589108d502963f879cb fs: fully initialize _OVERLAPPED for win32 (Cory Fields)
Pull request description:
```bash
fs.cpp: In member function ‘bool fsbridge::FileLock::TryLock()’:
fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::InternalHigh’ [-Werror=missing-field-initializers]
129 | _OVERLAPPED overlapped = {0};
| ^
fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::<anonymous>’ [-Werror=missing-field-initializers]
fs.cpp:129:32: error: missing initializer for member ‘_OVERLAPPED::hEvent’ [-Werror=missing-field-initializers]
```
Came up in #25972. That PR is now rebased on this change.
Closes: #26006
ACKs for top commit:
sipsorcery:
tACK 02c9e564687af6ae2b0b6589108d502963f879cb.
hebasto:
ACK 02c9e564687af6ae2b0b6589108d502963f879cb, tested on Linux x86_64:
Tree-SHA512: 6a0495c34bd952b2bb8c994a1450da7d3eee61225bb4ff0ce009c013f5e29dba94bb1c3ecef9989dc18c939909fdc8eba690a38f96da431ae9d64c23656de7d0
8ed2b72767de55dce033d8bfe6f9414ae14e1452 qt: Prevent wrong handling of `%2` token by Transifex (Hennadii Stepanov)
Pull request description:
On master (124e75a41ea0f3f0e90b63b0c41813184ddce2ab), Transifex translation check fails for 124e75a41e/src/qt/forms/intro.ui (L206) with a message:
> The expression '%2G' is not present in the translation.
In "Organization Settings" --> ["Translation checks"](https://www.transifex.com/bitcoin/settings/validations/) I have changed the status of the "**Variable substitution specifiers (like "%s") are preserved in the translations.**" check from "error" to "warning" temporarily. This setting should be reverted after applying this PR change.
[Noted](https://www.transifex.com/bitcoin/bitcoin/translate/#ru/qt-translation-024x/436102928/) by Transifex user [AHOHNMYC](https://www.transifex.com/user/profile/AHOHNMYC/).
I faced the same issue while working on Ukrainian translation.
ACKs for top commit:
katesalazar:
ACK 8ed2b72767de55dce033d8bfe6f9414ae14e1452
jarolrod:
ACK 8ed2b72767de55dce033d8bfe6f9414ae14e1452
Tree-SHA512: 304f795ac9241ac8453c614ed18d967226d9d515f9ea079b51af5bcbe2f0760ca7dcaea5efb38207720cb7a18159c2bcd337b961bc522a128715c70e0db81061
4f67336f1105b7c34a9e8cdafa603edc1d899fb9 test: verify best blockhash after invalidating an unknown block (brunoerg)
Pull request description:
Fixes#26051
Verify the best blockhash is the same after invalidating an unknown block, not the whole `getchaintip` response.
ACKs for top commit:
instagibbs:
ACK 4f67336f1105b7c34a9e8cdafa603edc1d899fb9
Tree-SHA512: 2d71743c1d3a317ef7b750f88437df71d1aed2728d9edac8b763a343406e168b97865ab25ec4c89caf09d002e076458376618cbd0845496375f7179633c88af9
644772b9efffda4dac01aff54042b3162079514d message-capture-parser: fix AssertionError on parsing `headers` message (Sebastian Falbesoner)
Pull request description:
If a test framework message's field name is in the list of `HASH_INT_VECTORS`, we currently assume that it _always_ has to contain a vector of integers and throw otherwise:
0ebd4db32b/contrib/message-capture/message-capture-parser.py (L82-L83)
(introduced in PR #25367, commit 42bbbba7c83d1e2baad18b4c6f05bad1358eb117).
However, that assumption is too strict. The (de)serialization field name "headers" is used in two different message types, one for `cfcheckpt` (where it is serialized as an integer vector), and another time for `headers` (where it is serialized as a vector of `CBlockHeader`s). Parsing the latter fails as it is not an integer vector and thus triggers the assert.
Fix this by adding the integer type check as additional condition to the `HASH_INT_VECTORS` check rather than asserting.
Fixes#25954.
ACKs for top commit:
glozow:
ACK 644772b9efffda4dac01aff54042b3162079514d
Tree-SHA512: c98a107f6703c6c1a81771907c25bcc171c631b57fd605fbebaedd93d651e2ef02fb5601853a9bc7d659ab531c5f47770181173a36ea2b37f584aa7a37b66505
4b1d5a10537ab48e3457606ba1cf2ae26a1cb2b2 test: invalidating an unknown block throws an error (brunoerg)
Pull request description:
While playing with `invalidateblock`, I unintentionally tried to invalidate an unknown block and it threw an error. Looking at the tests I just realized there is no test coverage for this case. This PR adds it.
Top commit has no ACKs.
Tree-SHA512: 25286ead809b3ad022e759127ef3134b271fbe76cb7b50ec2b0c7e2409da8d1b01dc5e80afe73e4564cc9c9c03487a1fe772aea3456988552d2f9c8fb34c730b
19db5875f0 Merge bitcoin/bitcoin#25959: doc: Fix link to MurmurHash3.cpp (moved from Google Code to Github) (MacroFake)
676eb1b8d7 Merge bitcoin/bitcoin#25967: refactor: add LIFETIMEBOUND to blockfilter where needed (MacroFake)
82116f2c07 Merge bitcoin/bitcoin#25883: doc: Security config warning (MacroFake)
1c678a1b62 Merge bitcoin/bitcoin#25888: refactor: use `strprintf` for creating unknown-service-flag string (MacroFake)
1526885baf Merge bitcoin/bitcoin#25836: subtree: update crc32c subtree (MacroFake)
c9d3695688 Merge bitcoin/bitcoin#25798: build: fix cleanup of test logs (Andrew Chow)
afc1a9f4ac Merge bitcoin/bitcoin#25811: doc: test: suggest multi-line imports in functional test style guide (MacroFake)
3e693ddfb5 Merge bitcoin/bitcoin#25788: guix: patch NSIS to remove .reloc sections from installer stubs (Andrew Chow)
f8719ec4a4 Merge bitcoin/bitcoin#22176: test: Correct outstanding -Werror=sign-compare errors (fanquake)
8fcd549956 Merge bitcoin/bitcoin#22180: fuzz: Increase branch coverage of the float fuzz target (MarcoFalke)
Pull request description:
## What was done?
Trivial Backports
## How Has This Been Tested?
Built locally; did not run tests or review
## 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
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: 457a69ced1684f550fe80a2456ae828337d236a47f2b6e8af2f395698877a1f7daaaadb32fdfc9d338009a90568c7bddbc4f7d172e238840555c9613fe5fb18f
2c05dc7811db4fe61d7f30fc9b46c374f75226c5 Fix link to MurmurHash3.cpp from Austin Appleby (dontbyte)
Pull request description:
Google Code repo doesn't exist anymore
ACKs for top commit:
Zero-1729:
crACK 2c05dc7811db4fe61d7f30fc9b46c374f75226c5
Tree-SHA512: 3e095255757b536f382ffb63e4292413592246c2446d486acbb71c52e4a3ece519d7cfae941685d9e25fd62de5c783510b3d076cd990a3d391496dc3076a0385
89576ccc572fcaf9fb7117ad6124482cc95fbd9f refactor: add LIFETIMEBOUND to blockfilter where needed (stickies-v)
Pull request description:
Noticed from https://github.com/bitcoin/bitcoin/pull/25637#issuecomment-1231860822 that [`BlockFilter::GetFilter()`](01e1627e25/src/blockfilter.h (L132)) returns a reference to a member variable. Added LIFETIMEBOUND to all blockfilter-related code to ensure that the return values do not have a lifetime that exceeds the lifetime of what it is bound to. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound or https://github.com/bitcoin/bitcoin/pull/25060 for a similar example.
I used `grep -E '[a-zA-Z>0-9][&*] ([a-zA-Z]*)\((.*)\)' src/**/blockfilter*` to grep all possible occurrences (not all of them require LIFETIMEBOUND)
ACKs for top commit:
brunoerg:
crACK 89576ccc572fcaf9fb7117ad6124482cc95fbd9f
Tree-SHA512: 6fe61fc0c1ed9e446edce083d1b093e1a5e2ef8c39ff74125bb12a24e514d45711845809817fbd4a04d7a9c23c8b362203771c17b6d831d2560b1af268453019
706c8e096978b694fb56dee9e8ed8f55373ad5a0 refactor: use `strprintf` for creating unknown-service-flag string (Sebastian Falbesoner)
Pull request description:
No need to use a stringstream here. The trivial change can be verified by running the functional test `rpc_net.py`:
c73c8d53fe/test/functional/rpc_net.py (L181-L184)
As far as I could tell, this is the only instace left where we used `std::ostringstream` for the creation of simple strings (in `FormatSubVersion` using a stream makes sense since the number of placeholders is not constant).
ACKs for top commit:
kristapsk:
ACK 706c8e096978b694fb56dee9e8ed8f55373ad5a0
Tree-SHA512: 069cea29aef03996ae16a0dc3ed87b1b2cf2ab0bf5987c225b10da12d0f4b62b7c3faf3a169c0b912eb2ad60c6ea0a09a622be7eaadad78cee0463ef4ffc0e19
4edc6893825fd8c45c53c81c73a6a7801e1b458c doc: test: suggest multi-line imports in functional test style guide (Sebastian Falbesoner)
Pull request description:
As long as I remember contributing to functional tests (~2-3 years), it was always kind of an unwritten rule that multi-line imports are preferred over single-line imports in order to reduce the possibility of potential merge conflicts -- at least if more than one symbol from a module is imported. This PR adds this rule to the style guide and adapts the example test accordingly. (Inspired by https://github.com/bitcoin/bitcoin/pull/25792#discussion_r941180819).
ACKs for top commit:
kouloumos:
ACK 4edc6893825fd8c45c53c81c73a6a7801e1b458c
1440000bytes:
ACK 4edc689382
w0xlt:
ACK 4edc689382
fanquake:
ACK 4edc6893825fd8c45c53c81c73a6a7801e1b458c
Tree-SHA512: c7b6ff62f601f4e57cc8334d291662987d6737ebca8d81c063280409f4412302172f1404ec16afc9a13007bcdba55bdab66b9b80363287e287888929cb386584
7a0b129c41d9fefdbc20d6d04983dd87bb8379e7 guix: patch NSIS to remove .reloc sections from install stubs (fanquake)
Pull request description:
With the release of binutils/ld 2.36, ld swapped to much improved
default settings when producing windows binaries with mingw-w64. One of
these changes was to stop stripping the .reloc section from binaries,
which is required for working ASLR.
When we switched to using a newer Guix time-machine in #23778, we begun
using binutils 2.37 to produce releases. Since then, our windows
installer (produced with makensis) has not functioned correctly when run on
a Windows system with the "Force randomization for images (Mandatory ASLR)"
option enabled. Note that all of our other release binaries, which all
contain .reloc sections, function fine under the same option, so it
cannot be just the presence of a .reloc section that is the issue.
The root cause of the problem is that when we compile NSIS (makensis), a number
of exe installer stubs are produced at the same time, for use later when makensis
is actually run. Given the new linker defaults, the stubs will contain .reloc sections,
when previously they would not. It seems that, in combination with how makensis
mutates the stub when it actually builds the installer, causes the problem.
According to upstream, https://sourceforge.net/p/nsis/bugs/1131/#abb6:
> Looks like the problem is the very existance of the .reloc section.
> It's not supposed to be there, and makensis doesn't handle it.
The most recent .reloc related upstream activity is in
https://sourceforge.net/p/nsis/bugs/1283/, where the conclusion again seemed to
be that .relo sections are not wanted, but there hasn't been any further follow up.
For now, restore pre-binutils-2.36 behaviour, by passing `-Wl,--disable-reloc-section`
to the linker when building the installer stubs, which fixes the produced installer.
The underlying issue can be further investigated in future.
.reloc section stripping is something we've accounted for previously,
see #18702, and related upstream discussion is in this thread:
https://sourceware.org/bugzilla/show_bug.cgi?id=19011.
Fixes#25726.
Guix Build (x86_64):
```bash
7e0723388913ac1ec9f650b943c6b23351ba0cd921c0ec830abf16b16724d503 guix-build-7a0b129c41d9/output/dist-archive/bitcoin-7a0b129c41d9.tar.gz
c3bb9c68895ffafa2900b0d18c1268e299d012a7dc70593f20f9900cf116eb05 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/SHA256SUMS.part
b57aa99c242b0aae64653c64ada38f6d3f0cbd902bbc096d3dc529fdcf87d681 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-debug.zip
341d99afc9961299883be6cd9666e8bc0f3f6296cff758719a32d27419acad36 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-setup-unsigned.exe
1d9ef48d3c9ed93a925962356b41cdaeb9d09fd758de193cd4d5f4d1ec6791eb guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-unsigned.tar.gz
28c81d99a9a4bd6648449393f91db213369e958add579ba9e9a1721540d2c4f7 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64.zip
```
Guix Build (arm64):
```bash
7e0723388913ac1ec9f650b943c6b23351ba0cd921c0ec830abf16b16724d503 guix-build-7a0b129c41d9/output/dist-archive/bitcoin-7a0b129c41d9.tar.gz
c3bb9c68895ffafa2900b0d18c1268e299d012a7dc70593f20f9900cf116eb05 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/SHA256SUMS.part
b57aa99c242b0aae64653c64ada38f6d3f0cbd902bbc096d3dc529fdcf87d681 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-debug.zip
341d99afc9961299883be6cd9666e8bc0f3f6296cff758719a32d27419acad36 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-setup-unsigned.exe
1d9ef48d3c9ed93a925962356b41cdaeb9d09fd758de193cd4d5f4d1ec6791eb guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64-unsigned.tar.gz
28c81d99a9a4bd6648449393f91db213369e958add579ba9e9a1721540d2c4f7 guix-build-7a0b129c41d9/output/x86_64-w64-mingw32/bitcoin-7a0b129c41d9-win64.zip
```
ACKs for top commit:
achow101:
ACK 7a0b129c41d9fefdbc20d6d04983dd87bb8379e7
hebasto:
ACK 7a0b129c41d9fefdbc20d6d04983dd87bb8379e7
jarolrod:
ACK 7a0b129c41d9fefdbc20d6d04983dd87bb8379e7
Tree-SHA512: 9e14e98207d20236b833603319fc4bb335c878a7c179ab495b33d143e2a900c6926125536bbb7499ee4f0f676cd5ea45c8c86cd7e544ed9a76bb298f98db6197
4e44f5bac4481d49ac53c458dcc5ca48e8b28414 test: Correct outstanding -Werror=sign-compare errors (Ben Woosley)
Pull request description:
I'm unclear on why these aren't failing on CI, but they failed for me locally, e.g.:
```
In file included from /usr/local/include/boost/test/test_tools.hpp:46:
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned int, int>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned int, int>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned int, int>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:153:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/streams_tests.cpp:122:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned int, int>' requested here
BOOST_CHECK_EQUAL(varint, 54321);
^
/usr/local/include/boost/test/tools/old/impl.hpp:107:17: error: comparison of integers of different signs: 'const unsigned long long' and 'const long' [-Werror,-Wsign-compare]
return left == right;
~~~~ ^ ~~~~~
/usr/local/include/boost/test/tools/old/impl.hpp:130:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl<unsigned long long, long>' requested here
return equal_impl( left, right );
^
/usr/local/include/boost/test/tools/old/impl.hpp:145:16: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::call_impl<unsigned long long, long>' requested here
return call_impl( left, right, left_is_array() );
^
/usr/local/include/boost/test/tools/old/impl.hpp:92:50: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::equal_impl_frwd::operator()<unsigned long long, long>' requested here
BOOST_PP_REPEAT( BOOST_TEST_MAX_PREDICATE_ARITY, IMPL_FRWD, _ )
^
/usr/local/include/boost/preprocessor/repetition/repeat.hpp:30:26: note: expanded from macro 'BOOST_PP_REPEAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:22:32: note: expanded from macro 'BOOST_PP_CAT'
^
/usr/local/include/boost/preprocessor/cat.hpp:29:34: note: expanded from macro 'BOOST_PP_CAT_I'
^
<scratch space>:161:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/serfloat_tests.cpp:41:5: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, unsigned long long, long>' requested here
BOOST_CHECK_EQUAL(TestDouble(std::numeric_limits<double>::infinity()), 0x7ff0000000000000);
^
ACKs for top commit:
theStack:
ACK 4e44f5bac4481d49ac53c458dcc5ca48e8b28414
Tree-SHA512: 8d9e5245676c61207ceacdf78c78a78ccc9fd2a2551d4d8df023513795591334aa2f5e1f4a2a8ed2bfeb381f1e226b6ba84c07e0de29a1f3f00da71f3a257bc1
fa13f34bf35129b38af699a0faf32c39d2ba8576 fuzz: Increase branch coverage of the float fuzz target (MarcoFalke)
fad0c58c3ecdf2a2a602ff39c9fd9dda7f8747d9 fuzz: Remove confusing return keyword from CallOneOf (MarcoFalke)
Pull request description:
Currently the branch coverage for the float fuzz target is only 50% : https://marcofalke.github.io/btc_cov/fuzz.coverage/src/test/fuzz/float.cpp.gcov.html
This is caused by the Fuzzed Data Provider only picking "nice" floats.
ACKs for top commit:
practicalswift:
cr ACK fa13f34bf35129b38af699a0faf32c39d2ba8576: patch looks correct
Tree-SHA512: 326822515e9a1c77647d41eab9a96185a3b320914d9264730fa72ffb76c2bf3dc5bf72cf6cd9beef14f4f032358d76a976860bf3e2418ae61943cf926c0ea086
1821d92b66 test: add activate_ehf_by_name (Alessandro Rezzi)
de38dca242 feat(consensus): Generalize ehf activation (Alessandro Rezzi)
Pull request description:
## Issue being fixed or feature implemented
Try to sign/mine any ehf deployment and not only mn_rr. As asked in the review this decouples (and improves) commit [e24cb23](e24cb239f8) from PR #5799
## What was done?
See commit description
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] 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
Top commit has no ACKs.
Tree-SHA512: 7112a5214b473dea29a892563e6598eca089d2e17fdff8bda08c7777738f1054d2cb07e0a7dccf66214911fec64a6441e84100273ac2ed52abe1d33fb960e8cc
7b3756f8c9 fix: rename "Mask values" to "Discreet mode" (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
As [noticed by](https://github.com/dashpay/dash/pull/5796#issuecomment-1874256010) thephez "Discreet mode" is a good name, Legder and Trezor use that.
## What was done?
Renamed qt option "Mask values" to "Discreet mode", update hot-key from Ctrl+Shift+M to Ctrl+Shift+D
## How Has This Been Tested?
Build & run qt app
## Breaking Changes
N/A, that's just gui text field.
## Checklist:
- [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
Top commit has no ACKs.
Tree-SHA512: 515c9c8caab02c78d6edb3859608bd6058dff04e2687567014ad721d2cff255e24bf4dd429dd18b5cac7f6a0f72415a5750fc0590dd6be05d2733ae438f60039
f44c07f75d feat: enable optional rebasing as part of github-merge.py script (pasta)
Pull request description:
## Issue being fixed or feature implemented
this will allow us to rebase the PR before merging as we do now; and then right after merge it into develop and push
note we can not simply rebase locally before merging as we would then violate the "all changes must be done through PR rule"
When rebasing locally; we also check the range-diff (should be all >'s indicating a commit being pulled in from the rebase and ='s indicating the commits are the same as the base PR. This ensures that when we force push we will not invalidate previously created reviews.
## What was done?
## How Has This Been Tested?
Rebased and Merged PR with it
## 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: 020be8f2451d8945ca10b6388d8bffea0736c43eb410b3f0d9cb5dc07eefd661d8d8c9f0a749ed0accd8288c2fb161f379408022eb2590bd50fa55c0145e072c
this will allow us to rebase the PR before merging as we do now; and then right after merge it into develop and push
note we can not simply rebase locally before merging as we would then violate the "all changes must be done through PR rule"
When rebasing locally; we also check the range-diff (should be all >'s indicating a commit being pulled in from the rebase and ='s indicating the commits are the same as the base PR. This ensures that when we force push we will not invalidate previously created reviews.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
2b26a87874 merge bitcoin#28012: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization (Kittywhiskers Van Gogh)
4eeafa267c partial bitcoin#27927: Allow std::byte and char Span serialization (Kittywhiskers Van Gogh)
cb2fa8360a test: place the std::ostream operator<< definition in namespace std (Kittywhiskers Van Gogh)
cf4522f845 partial bitcoin#23595: Add ParseHex<std::byte>() helper (Kittywhiskers Van Gogh)
e4091aa477 partial bitcoin#25296: Add DataStream without ser-type and ser-version (Kittywhiskers Van Gogh)
eab031a9b0 merge bitcoin#26258: Remove unused CDataStream::rdbuf method (Kittywhiskers Van Gogh)
95b5850434 partial bitcoin#25001: Modernize util/strencodings and util/string: string_view and optional (Kittywhiskers Van Gogh)
5fe72bbb8e merge bitcoin#24231: Fix read-past-the-end and integer overflows (Kittywhiskers Van Gogh)
24af37256f merge bitcoin#24253: Remove broken and unused CDataStream methods (Kittywhiskers Van Gogh)
baf8dd65cd merge bitcoin#24190: Fix sanitizer suppresions in streams_tests (Kittywhiskers Van Gogh)
e933d78a88 merge bitcoin#23438: Use spans of std::byte in serialize (Kittywhiskers Van Gogh)
d3b282208b merge bitcoin#23653: Generalize/simplify VectorReader into SpanReader (Kittywhiskers Van Gogh)
2c32a09f4e merge bitcoin#21969: Switch serialize to uint8_t (Kittywhiskers Van Gogh)
0a08dbf3f4 merge bitcoin#21824: Replace deprecated char with uint8_t in serialization (Kittywhiskers Van Gogh)
d0b4e560a6 merge bitcoin#21966: Remove double serialization; use software encoder for fee estimation (Kittywhiskers Van Gogh)
1d6aafea47 merge bitcoin#21817: Replace &foo[0] with foo.data() (Kittywhiskers Van Gogh)
d9a8ce2749 trivial: move GetSerializeSize away from Stream (Un)serialize functions (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/5902
* [bitcoin#24231](https://github.com/bitcoin/bitcoin/pull/24231) is merged after [bitcoin#24253](https://github.com/bitcoin/bitcoin/pull/24253) due to a MinGW bug ([comment](https://github.com/bitcoin/bitcoin/pull/24231#issuecomment-1031179638))
* [bitcoin#25001](https://github.com/bitcoin/bitcoin/pull/25001) is listed as unmerged despite being committed upstream as 132d5f8c2f
* [bitcoin#25296](https://github.com/bitcoin/bitcoin/pull/25296) is listed as unmerged despite being committed upstream as 79e007d1d6
* [bitcoin#21966](https://github.com/bitcoin/bitcoin/pull/21966) was partially backported in [dash#4197](https://github.com/dashpay/dash/pull/4197) as f946c68f83, including only 2be4cd94f4c7d92a4287971233a20d68db81c9c9.
The excluded commits have been backported, marking the pull request as fully merged.
* [bitcoin#23438](https://github.com/bitcoin/bitcoin/pull/23438) was partially backported in [dash#5574](https://github.com/dashpay/dash/pull/5574) as de54b8784c, including only fa65bbf.
The excluded commits have been backported, marking the pull request as fully merged.
* [bitcoin#27927](https://github.com/bitcoin/bitcoin/pull/27927) opened a fresh can of hell thanks to being (possibly?) the first pull request to include `std::byte` `BOOST_CHECK`'s to the unit test suite. For reasons still unbeknownst to me, it refused to compile, despite being perfectly happy when checked-out as a commit directly and built as-is from upstream.
The compile error was like this (edited for brevity):
```
CXX test/test_dash-serialize_tests.o
In file included from test/serialize_tests.cpp:13:
In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/unit_test.hpp:18:
In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/test_tools.hpp:46:
In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/old/impl.hpp:24:
/src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/detail/print_helper.hpp:53:13: error: static assertion failed due to requirement 'boost::has_left_shift<std::basic_ostream<char, std::char_traits<char>>, std::byte, boost::binary_op_detail::dont_care>::value': Type has to implement operator<< to be printable
BOOST_STATIC_ASSERT_MSG( (boost::has_left_shift<std::ostream,T>::value),
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[...]
<scratch space>:206:1: note: expanded from here
BOOST_PP_REPEAT_1
^
test/serialize_tests.cpp:347:9: note: in instantiation of function template specialization 'boost::test_tools::tt_detail::check_frwd<boost::test_tools::tt_detail::equal_impl_frwd, std::byte, std::byte>' requested here
BOOST_CHECK_EQUAL(out.at(0), std::byte{'a'});
^
[...]
In file included from test/serialize_tests.cpp:13:
In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/unit_test.hpp:18:
In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/test_tools.hpp:46:
In file included from /src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/old/impl.hpp:24:
/src/dash/depends/x86_64-pc-linux-gnu/include/boost/test/tools/detail/print_helper.hpp:55:18: error: invalid operands to binary expression ('std::ostream' (aka 'basic_ostream<char>') and 'const std::byte')
ostr << t;
~~~~ ^ ~
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/cstddef:130:5: note: candidate function template not viable: no known conversion from 'std::ostream' (aka 'basic_ostream<char>') to 'byte' for 1st argument
operator<<(byte __b, _IntegerType __shift) noexcept
^
[...]
5 warnings and 2 errors generated.
make[2]: *** [Makefile:17842: test/test_dash-serialize_tests.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/src/dash/src'
make[1]: *** [Makefile:18525: all-recursive] Error 1
make[1]: Leaving directory '/src/dash/src'
make: *** [Makefile:802: all-recursive] Error 1
```
* No such error was present on Bitcoin.
It is true, that no `std::ostream& operator<<(std::ostream& a, const std::byte& b)` is present by default but attempting to `grep` for any specializations didn't show anything _relevant_ that Dash didn't have. Searching on GitHub didn't help either.
* Then, assuming that perhaps Boost's assertion logic may have changed, upgraded the version of Boost to match the pull request at the time, Boost 1.81. That also did not do anything (and actually, caused a trailing slashes unit test to fail but doesn't cause any problem in Bitcoin because they got rid of their `boost::filesystem` usage by then).
* If that isn't it, then let's try building Bitcoin with Dash's depends. It built successfully, ran successfully. The problem isn't in the dependency, it's in the codebase.
* Since it seemed to be `std::byte` related, pull requests that are related to `std::byte` serialization were backported and `std::byte` serialization related changes needed for [dash#5902](https://github.com/dashpay/dash/pull/5902) were cherry-picked. That's why this pull request came to be. But it didn't help this particular issue (though it did smooth out the cherry-picks).
* Running out of ideas, `gdb` is used to step through `serialize_tests`'s `class_methods` and understand why `BOOST_CHECK_EQUAL(out.at(0), std::byte{'a'});` is valid in Bitcoin but not Dash by finding the elusive `operator<<`. This is where things go from bad to worse.
Turns out, when you build with `clang`, `gdb` loses the ability to do breakpoints by file. So, an attempt is made to use `lldb` (which btw, is called `lldb-16`, running `lldb` with yield an error if you're using the develop container) and it refuses to work, erroring `personality set failed: Operation not permitted`.
Turns out, the [`docker-compose.yml`](37f43e4e56/contrib/containers/develop/docker-compose.yml) needs the following additions:
```
cap_add:
- SYS_PTRACE
security_opt:
- "seccomp:unconfined"
```
After making these changes, `lldb` works and then we resume trying to find `operator<<`. After too many hours and nimbly alternating between `next` and `step`, tried making a return to `gdb` (compiling with `gcc` this time with the appropriate `CXXFLAGS`) hoping for different results and a while later, realized that it cannot step through Boost's headers (it doesn't recognize the filenames) and then recompile it with `clang` and return to `lldb`.
This was a wild goose chase.
* After a lot of futile efforts to find the operator by stepping through `BOOST_CHECK_EQUAL`, a basic example addressing the static assertion (that a left shift operator must exist of `<type>` (here `std::byte`) for `std::ostream`) was added in
```c++
std::ostream& ostr = std::cout;
ostr << std::byte{'a'};
```
...and it compiled in both codebases.
So the left shift that Boost is asserting doesn't exist does exist but it isn't being detected for some reason. Upon hovering the `<<`, VSCode highlighted the source of the definition as [`setup_common.h`](96ac317c27/src/test/util/setup_common.h (L27-L32)) thanks to the comment above it.
Diffing between Bitcoin and Dash revealed the secret, the `operator<<` definition was placed under namespace `std` by [bitcoin#23497](https://github.com/bitcoin/bitcoin/pull/23497) in f7086fd8ff (see [change](f7086fd8ff/src/test/util/setup_common.h (L29-L35))).
That change has now been made in a separate commit.
## Breaking Changes
Changes in serialization APIs will make backports predating [bitcoin#23438](https://github.com/bitcoin/bitcoin/pull/23438) annoying but will _not_ change how data is stored on disk.
## 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: 1d7c67f5fe282f78e24cb720828e5f1f48b6926006b903c28399938532cc5c470c175b00c8b80e9662c4467c1201e09ae6d1cd9b95e8b20ace5e4410c72c472e
