💬 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?

💬 fjahr commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727686414)
And why remove this?

👍 hodlinator approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2255475669)
re-ACK 81554aac80bf2270db977c110c37acc7e8034194

`git range-diff master cf88a55e97719aabd62f0b608df0800fef8304de 81554aac80bf2270db977c110c37acc7e8034194`

Only changes `BOOST_AUTO_TEST_CASE(from_user_hex)` tests, error strings, commit messages and revives the `LogPrintf` -> `LogInfo` update.

💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1727696360)
nit: Inadvertent newline removal?

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1727703220)
Better than my suggestion, thanks!

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727707662)
> 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?

Thanks for the review! I aimed to view the code from the perspective of layman Bitcoin enthusiasts, trying to get an answer to their question, "How do we know the limit isn't greater than 21 million?".
I prioritized a high signal-to-noise ratio, omitting checks already covered by other tests. This test was about validating limits, not `GetBlock
...

💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727707982)
Please see: https://github.com/bitcoin/bitcoin/pull/30699#discussion_r1727707662