💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-2324533392)
Closing this again, having worked with the code some more over the past year, I don't think this is particularly pressing change anymore. As ryanofsky elaborated, our context objects should have functionality and instead just hold state - which is what it does already.
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-2324533392)
Closing this again, having worked with the code some more over the past year, I don't think this is particularly pressing change anymore. As ryanofsky elaborated, our context objects should have functionality and instead just hold state - which is what it does already.
💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2324555343)
Tested dc08930ec68b465420cc5e1157e532fcb5cf8c87 again on Windows.
x86_64 guix hashes
```
01d11755aaf73fd378b563afa5f54286fbe9c8e2a3b6259a61d21b7cbb5bf9f4 guix-build-dc08930ec68b/output/aarch64-linux-gnu/SHA256SUMS.part
2baf27e4f865ed23b97d7f353bbcf6f8fd5629c8d7cec26ee02b048406d3418b guix-build-dc08930ec68b/output/aarch64-linux-gnu/bitcoin-dc08930ec68b-aarch64-linux-gnu-debug.tar.gz
a686ed4a50b371a3ef2d20791e4fdf1f1258404f1012efc132a906808835628b guix-build-dc08930ec68b/output/aarch64
...
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2324555343)
Tested dc08930ec68b465420cc5e1157e532fcb5cf8c87 again on Windows.
x86_64 guix hashes
```
01d11755aaf73fd378b563afa5f54286fbe9c8e2a3b6259a61d21b7cbb5bf9f4 guix-build-dc08930ec68b/output/aarch64-linux-gnu/SHA256SUMS.part
2baf27e4f865ed23b97d7f353bbcf6f8fd5629c8d7cec26ee02b048406d3418b guix-build-dc08930ec68b/output/aarch64-linux-gnu/bitcoin-dc08930ec68b-aarch64-linux-gnu-debug.tar.gz
a686ed4a50b371a3ef2d20791e4fdf1f1258404f1012efc132a906808835628b guix-build-dc08930ec68b/output/aarch64
...
💬 Sjors commented on issue "UpdateTime() needs to account for timewarp fix on testnet4 ":
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324563969)
This was fixed in #30681.
However someone should have tried a full IBD before changing the testnet4 consensus rules. An incompatible block in the past won't be retroactively rejected by miners, but it will be by new noes.
I'll try that, and open a new issue if needed.
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324563969)
This was fixed in #30681.
However someone should have tried a full IBD before changing the testnet4 consensus rules. An incompatible block in the past won't be retroactively rejected by miners, but it will be by new noes.
I'll try that, and open a new issue if needed.
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740791280)
I'm not sure if it makes sense adding it to this branch, because I think the tests should be removed after switching `ParseHashV` to use `uint256::FromHex()` so that would probably create a bit too much unnecessary churn, but here's a commit (to which you can easily add additional inputs) you can cherry-pick that verifies the parity of both functions:
https://github.com/stickies-v/bitcoin/commit/1f2b0fa86db2ed65476b68417aa1bf4c26026a19
> (Q: what should happen when v.isStr() is false?)
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740791280)
I'm not sure if it makes sense adding it to this branch, because I think the tests should be removed after switching `ParseHashV` to use `uint256::FromHex()` so that would probably create a bit too much unnecessary churn, but here's a commit (to which you can easily add additional inputs) you can cherry-pick that verifies the parity of both functions:
https://github.com/stickies-v/bitcoin/commit/1f2b0fa86db2ed65476b68417aa1bf4c26026a19
> (Q: what should happen when v.isStr() is false?)
...
📝 marcofleon opened a pull request: "doc: fix compiler flags for macOS configuration"
(https://github.com/bitcoin/bitcoin/pull/30785)
Small CMake correction in the macOS build docs.
(https://github.com/bitcoin/bitcoin/pull/30785)
Small CMake correction in the macOS build docs.
💬 hodlinator commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740800465)
Being able to write `BOOST_CHECK_EQUALS(..., std::nullopt);`, in case one wanted to.
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740800465)
Being able to write `BOOST_CHECK_EQUALS(..., std::nullopt);`, in case one wanted to.
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740800983)
> I'm not a big fan of expressing code structure with dead comments
I agree, but I'm not sure this is a dead comment? I don't think `hex` would be a better parameter name, because it's not validated to be a hex string yet, could be anything. The "64 hex digit" bit helps developers that want to use this utility function quickly understand how to use it, as does the "most significant digits first" bit.
I'd be okay with not having the string too, but it feels rather arbitrary so I'd rather
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740800983)
> I'm not a big fan of expressing code structure with dead comments
I agree, but I'm not sure this is a dead comment? I don't think `hex` would be a better parameter name, because it's not validated to be a hex string yet, could be anything. The "64 hex digit" bit helps developers that want to use this utility function quickly understand how to use it, as does the "most significant digits first" bit.
I'd be okay with not having the string too, but it feels rather arbitrary so I'd rather
...
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324590773)
Looks not using `xxd` fixed it, thanks @maflcko !
> Seems a bit odd to implement this both in Python and cmake (by calling xxd). Wouldn't it be easier to just implement it once and require that to be called before use in the other place?
Yeah, I first implemented it in python, then autotools and finally cmake. I kept the python version around because I wasn't sure which approach we might actually want to go (see https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2318625694) and bec
...
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324590773)
Looks not using `xxd` fixed it, thanks @maflcko !
> Seems a bit odd to implement this both in Python and cmake (by calling xxd). Wouldn't it be easier to just implement it once and require that to be called before use in the other place?
Yeah, I first implemented it in python, then autotools and finally cmake. I kept the python version around because I wasn't sure which approach we might actually want to go (see https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2318625694) and bec
...
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740803786)
Now using the python script from cmake, so marking as resolved.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740803786)
Now using the python script from cmake, so marking as resolved.
💬 hodlinator commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2324596355)
> > TIL: one can compare `optional` against a value without `.value()` (C++17 addition).
>
> Yeah, but I don't think that's used here
It is used in your change from
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
```
to
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);
```
It was my obscure way of saying thank you for making me discover that. :)
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2324596355)
> > TIL: one can compare `optional` against a value without `.value()` (C++17 addition).
>
> Yeah, but I don't think that's used here
It is used in your change from
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
```
to
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);
```
It was my obscure way of saying thank you for making me discover that. :)
💬 brunoerg commented on pull request "doc: fix compiler flags for macOS configuration":
(https://github.com/bitcoin/bitcoin/pull/30785#discussion_r1740808306)
Perhaps this phrase should be changed as well since there is no `configure` command anymore?
(https://github.com/bitcoin/bitcoin/pull/30785#discussion_r1740808306)
Perhaps this phrase should be changed as well since there is no `configure` command anymore?
🤔 stratospher reviewed a pull request: "test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py"
(https://github.com/bitcoin/bitcoin/pull/30761#pullrequestreview-2275469312)
ACK fa247e6. Checked the timeout downloading block logs before/after using `setmocktime`.
(https://github.com/bitcoin/bitcoin/pull/30761#pullrequestreview-2275469312)
ACK fa247e6. Checked the timeout downloading block logs before/after using `setmocktime`.
✅ Sjors closed an issue: "UpdateTime() needs to account for timewarp fix on testnet4 "
(https://github.com/bitcoin/bitcoin/issues/30614)
(https://github.com/bitcoin/bitcoin/issues/30614)
💬 Sjors commented on issue "UpdateTime() needs to account for timewarp fix on testnet4 ":
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324609533)
> However someone should have tried a full IBD before changing the testnet4 consensus rules.
I just did. It gets stuck at block 42,335 from yesterday. So at least there was no historical block violating the rule at the time this "soft fork" was introduced.
Opening a fresh issue...
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324609533)
> However someone should have tried a full IBD before changing the testnet4 consensus rules.
I just did. It gets stuck at block 42,335 from yesterday. So at least there was no historical block violating the rule at the time this "soft fork" was introduced.
Opening a fresh issue...
💬 TheCharlatan commented on pull request "[refactor] Cleanup BlockAssembler mempool usage":
(https://github.com/bitcoin/bitcoin/pull/28843#issuecomment-2324628138)
Reworked a323388036d4f8b9fe8ff0915252f20abb91633e -> 247ae848e669cc36c790362b766ff813cdfa3e66 ([blockAssemblerRemoveMempool_1](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_1) -> [blockAssemblerRemoveMempool_2](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockAssemblerRemoveMempool_1..blockAssemblerRemoveMempool_2))
* Took @ajtowns's [suggestion](https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/28843#issuecomment-2324628138)
Reworked a323388036d4f8b9fe8ff0915252f20abb91633e -> 247ae848e669cc36c790362b766ff813cdfa3e66 ([blockAssemblerRemoveMempool_1](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_1) -> [blockAssemblerRemoveMempool_2](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockAssemblerRemoveMempool_1..blockAssemblerRemoveMempool_2))
* Took @ajtowns's [suggestion](https://github.com/bitcoin/bit
...
💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740836897)
Using a mutable C-style array seems a bit fragile. IIUC, starting with C++17, using `inline constexpr std::array ip_asn{...};` should allow the compiler to optimize the layout, as well as enforce immutability. Moreover, `std::array` drops the need for `ip_asn_len`, because the size is embedded in the type of the array. Unrelated, you can change the `0x{...}` above to `std::byte{...}` and then use `MakeUCharSpan` instead of legacy C-Style reinterpret cast and static cast in the code that uses the
...
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740836897)
Using a mutable C-style array seems a bit fragile. IIUC, starting with C++17, using `inline constexpr std::array ip_asn{...};` should allow the compiler to optimize the layout, as well as enforce immutability. Moreover, `std::array` drops the need for `ip_asn_len`, because the size is embedded in the type of the array. Unrelated, you can change the `0x{...}` above to `std::byte{...}` and then use `MakeUCharSpan` instead of legacy C-Style reinterpret cast and static cast in the code that uses the
...
⚠️ Sjors opened an issue: "Testnet4 consensus failure due to timewarp related "softfork""
(https://github.com/bitcoin/bitcoin/issues/30786)
#30647 was effectively a soft-fork on testnet4 with no activation plan.
It reduced the timewarp mitigation allowance from 7200 to 600 seconds.
It's enforced by folks running v28.0rc1 and recent master, but not by miners.
This causes new nodes to be stuck at block 42,335: https://mempool.space/testnet4/block/00000000542792e54a720567ba66157d48cdae7bfd01c1b678d0f07a2ed56e99
As observed on BitcoinTalk: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234 and pointed ou
...
(https://github.com/bitcoin/bitcoin/issues/30786)
#30647 was effectively a soft-fork on testnet4 with no activation plan.
It reduced the timewarp mitigation allowance from 7200 to 600 seconds.
It's enforced by folks running v28.0rc1 and recent master, but not by miners.
This causes new nodes to be stuck at block 42,335: https://mempool.space/testnet4/block/00000000542792e54a720567ba66157d48cdae7bfd01c1b678d0f07a2ed56e99
As observed on BitcoinTalk: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234 and pointed ou
...
💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324655223)
Note that the block that upgraded nodes will get stuck on (42,335) has difficulty 1. It was probably mined by setting the machine clock 20 minutes in the future. The original testnet4 rules allowed the next block to use the real time. But under the new rules it needs to be 10 minutes in the future (which #30681 takes care of).
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324655223)
Note that the block that upgraded nodes will get stuck on (42,335) has difficulty 1. It was probably mined by setting the machine clock 20 minutes in the future. The original testnet4 rules allowed the next block to use the real time. But under the new rules it needs to be 10 minutes in the future (which #30681 takes care of).
⚠️ maflcko opened an issue: "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time"
(https://github.com/bitcoin/bitcoin/issues/30787)
It looks like this step is taking longer than the entire `cmake -B` without it. Is there a reason why it takes so long, an whether it can be sped up?
(https://github.com/bitcoin/bitcoin/issues/30787)
It looks like this step is taking longer than the entire `cmake -B` without it. Is there a reason why it takes so long, an whether it can be sped up?
💬 hebasto commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298)
It compiles and links the minimal code that uses the `boost/test/included/unit_test.hpp` header. Verifying that the header simply exists could be an alternative.
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298)
It compiles and links the minimal code that uses the `boost/test/included/unit_test.hpp` header. Verifying that the header simply exists could be an alternative.