💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2305181520)
> Sorry for not understanding that you had it in the works with my prior suggestion.

No problem at all, I decided to give this PR a bit of a rest to focus on reviewing and aligning with related PRs, and think about the suggestions made here, hence the delay in updating this. Will quickly address feedback again from here on.

> The only part that still bugs me a bit is the naming of `FromUserHex` -> the "user" is on a different abstraction level, I'd prefer mentioning the behavior change com
...

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727474250)
re: https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1727105521

This is a good catch, but I"m not sure there is a observable change of behavior in `ActivateBestChain` because cs_main is held the whole time `blockTip` / `uiInterface.NotifyBlockTip` / `RPCNotifyBlockChange` is called and while the old `latestblock` global (now replaced by `g_best_block`) was updated:

https://github.com/bitcoin/bitcoin/blob/5ce2285b8739e12eebd1d53358038ec2277c662b/src/validation.cpp#L3552

So I don
...

📝 instagibbs opened a pull request: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698)
Basic addition to test case added in https://github.com/bitcoin/bitcoin/pull/30681

👋 instagibbs's pull request is ready for review: "test: Add time-timewarp-attack boundary cases"
(https://github.com/bitcoin/bitcoin/pull/30698)

💬 instagibbs commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2305206232)
I tested this patchset with 2016 target timespan and tests passed as expected. I also added a boundary test case: https://github.com/bitcoin/bitcoin/pull/30698

💬 jonatack commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#discussion_r1727515494)
I could be misreading, but it looks like all the tor and i2p peers have been removed without being added elsewhere.

💬 jonatack commented on pull request "[WIP] seeds: Add additional seed source and bump uptime requirements for Onion and I2P nodes":
(https://github.com/bitcoin/bitcoin/pull/30695#issuecomment-2305249422)
> As a result, its data contains additional eligible Onion (and other darknet nodes), as is shown in the histogram below.

Some very good tor and i2p peers seems absent from the update. I addnode them, but hm.

🤔 tdb3 reviewed a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658#pullrequestreview-2255241049)
post merge ACK 221809b81cfcecb04050915eebacffda2599da42

Checked chainparams (mainnet and signet) manually on each of my nodes. Matched.

```
$ bitcoin-cli getblockhash 856760
$ bitcoin-cli getchaintxstats 4096 <block hash from getblockhash>
$ bitcoin-cli getblockheader <block hash from getblockhash>
$ bitcoin-cli -signet getblockhash 208800
$ bitcoin-cli -signet getchaintxstats 4096 <block hash from getblockhash>
$ bitcoin-cli -signet getblockheader <block hash from getblockhash>
``
...

📝 l0rinc opened a pull request: "test: Improve clarity of subsidy limit test"
(https://github.com/bitcoin/bitcoin/pull/30699)
The test has been divided into two distinct parts to enhance transparency and build confidence, especially for layman reviewers, in verifying Bitcoin's monetary policy finality.

First, we iterate through every single block until the subsidy reaches zero, removing confusing jumps, hard-coded block limits, and intermediary assertions.

Then, we ensure the subsidy remains zero by checking up to block 10,000,000 (skipping every 1000 blocks for efficiency).

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2305343773)
Code review ACK 81554aac80bf2270db977c110c37acc7e8034194. Thanks for all the changes. This seems both simpler and more backward compatible now.

---

re: https://github.com/bitcoin/bitcoin/pull/30569#issue-2443189383

In PR description:

>* test: the optional `RANDOM_CTX_SEED` env var is now required to consist only of up to 64 hex digits (optionally prefixed with "0x"), otherwise the program will abort

I think it would be clearer and more consistent with the rest of the description i
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727515102)
In commit "refactor: add uint256::FromUserHex helper" (4e05d9459ad275d5929bab8f55e7edcd0be9dca9)

I found these two tests and especially the comment above "a value is expected when the last (65th) character is trimmed off valid_hex_65" confusing because they make it sound like the function is supposed to trim off the 65th hex character and ignore it.

Would change the comment above to plainly describe the string: `// 65 digit hex number with 0x prefix`

And add a comment to explain the thi
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727569784)
In commit "refactor: add uint256::FromUserHex helper" (4e05d9459ad275d5929bab8f55e7edcd0be9dca9)

We seem to be dropping the mixed case checks from the previous code:

```c++
BOOST_CHECK(IsHexNumber("0xFfa"));
BOOST_CHECK(IsHexNumber("Ffa"));
BOOST_CHECK(IsHexNumber("0x00112233445566778899aabbccddeeffAABBCCDDEEFF"));
BOOST_CHECK(IsHexNumber("00112233445566778899aabbccddeeffAABBCCDDEEFF"));
```

I tend to think it would be good to keep these, so code is changing less. B
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727558308)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1725693175
re: https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2305181520

Maybe I have a slightly different perspective because I think it can be good to name functions after how they are intended to be used, instead of what they do, depending on what's more relevant to callers. In this case, though, I do think the name describes what it does: parse hex input from a user. The assumption is that callers should not make i
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727507122)
In commit "refactor: add uint256::FromUserHex helper" (4e05d9459ad275d5929bab8f55e7edcd0be9dca9)

Not important but could also add a test that this does not allow spaces after a 64 digit string (not just a short string).

👍 ryanofsky approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2255352809)
Code review ACK 81554aac80bf2270db977c110c37acc7e8034194

Just split up the last commit since the previous review to separate bugfix from cleanup

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2305438162)
I reviewed everything from scratch commit-by-commit. I couldn't find any massive holes, only a few instances of missing stuff of severity such as https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726881676, which is trivial and quick to follow-up on and not a blocker. (My plan is to submit the review comments after merge, in one batch, so as to not distract this pull request right now with noise and comment-cycles)

However, I did ***not*** review:

* `cmake/module/Find{*}.cmake`
*
...

💬 fjahr commented on pull request "test: Add time-timewarp-attack boundary cases":
(https://github.com/bitcoin/bitcoin/pull/30698#issuecomment-2305446406)
Code review ACK 31378d44f44cabc576bf92ddd61afde4b528de77

🤔 hodlinator reviewed a pull request: "test: Improve clarity of subsidy limit test"
(https://github.com/bitcoin/bitcoin/pull/30699#pullrequestreview-2255459494)
Nice to check the total subsidy without relying on `nSubsidyHalvingInterval` being evenly divisible by 1000. Neat to have the height of the first zero-subsidy block.

I do not like that the test no longer checks against max subsidy size and overflow/unreasonableness of the sum. What is your reasoning around that?

The old test went on for ~2x the subsidy-granting height, seems like a limit as least as good as 10m?

---

The minimum improvement I would like to see in the old test would be
...

🤔 fjahr reviewed a pull request: "test: Improve clarity of subsidy limit test"
(https://github.com/bitcoin/bitcoin/pull/30699#pullrequestreview-2255460624)
Your changed version doesn't check the same things as the old version and overall this doesn't look like an improvement to me. I would be neutral on the change if you just add some comments, that should be enough to improve clarity. The refactoring doesn't seem necessary to improve clarity and given the importance of this test I don't think many will be keen to invest their time to review code changes here just for improving clarity.

💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727685598)
Why would you remove this check?