🚀 fanquake merged a pull request: "scripted-diff: LogPrint -> LogDebug"
(https://github.com/bitcoin/bitcoin/pull/30750)
(https://github.com/bitcoin/bitcoin/pull/30750)
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2275360087)
light code review and Tested ACK 100e1b9ecb29c76187112fda9fed1c01da192c99
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2275360087)
light code review and Tested ACK 100e1b9ecb29c76187112fda9fed1c01da192c99
🤔 ismaelsadeeq reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2275372899)
Post-merge Tested ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Tested on M2 Pro, Sonoma 14.1.1
Clang version 18.1.7
CMake version 3.29.5
Configuration and compilation completed smoothly without any issues.
Started bitcoind without any issue and tested a few RPCs.
I also ran unit tests and benchmarks, all ran smoothly.
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2275372899)
Post-merge Tested ACK 41051290ab3b6c36312cec26a27f787cea9961b4
Tested on M2 Pro, Sonoma 14.1.1
Clang version 18.1.7
CMake version 3.29.5
Configuration and compilation completed smoothly without any issues.
Started bitcoind without any issue and tested a few RPCs.
I also ran unit tests and benchmarks, all ran smoothly.
💬 garlonicon commented on issue "UpdateTime() needs to account for timewarp fix on testnet4 ":
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324516655)
If someone uses 600 seconds as a MAX_TIMEWARP, then that node can be stuck on block 42335. More information: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234
I wonder, if all blocks after 42335 will be reorged, or if MAX_TIMEWARP will be bigger than 600. I guess v28.0 was released with that value, and some nodes are stuck, because of that.
(https://github.com/bitcoin/bitcoin/issues/30614#issuecomment-2324516655)
If someone uses 600 seconds as a MAX_TIMEWARP, then that node can be stuck on block 42335. More information: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234
I wonder, if all blocks after 42335 will be reorged, or if MAX_TIMEWARP will be bigger than 600. I guess v28.0 was released with that value, and some nodes are stuck, because of that.
💬 hebasto commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1740764357)
Thanks! Your proposal has been accepted.
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1740764357)
Thanks! Your proposal has been accepted.
✅ TheCharlatan closed a pull request: "kernel: Run sanity checks on context construction"
(https://github.com/bitcoin/bitcoin/pull/28228)
(https://github.com/bitcoin/bitcoin/pull/28228)
💬 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...