💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907609405)
Ahh I see,
Initially I was under the impression that it did, but I was wrong hence I updated the title and description

🤔 furszy reviewed a pull request: "wallet: Cleanup accidental encryption keys in watchonly wallets"
(https://github.com/bitcoin/bitcoin/pull/28724#pullrequestreview-2537883870)
Code review ACK 69e95c2b4f99eb4

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1907617007)
Fixed in https://github.com/bitcoin/bitcoin/compare/bf5c569898d0297de010102a623bf52009607ed8..ea62aaed3b1851605c9ff33a9d9f9edcb53828a5

💬 pablomartin4btc commented on pull request "test: raise explicit error if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1907622464)
> +1. Something like this would be helpful. I've received a few messages sharing these logs without knowing what to do next.

I was of them :raising_hand:, I stepped upon this issue while testing #31451 and I found this [answer](https://bitcoin.stackexchange.com/questions/121085/how-can-i-run-the-skipped-bitcoin-functional-tests) from @achow101 in stack-overflow before checking [/test/README.md](https://github.com/bitcoin/bitcoin/blob/master/test/README.md).

I'd put a reference to the `READ
...

🤔 pablomartin4btc reviewed a pull request: "test: raise explicit error if any of the needed release binaries is missing"
(https://github.com/bitcoin/bitcoin/pull/31462#pullrequestreview-2537897406)
Concept ACK.

Left a few [suggestions](https://github.com/bitcoin/bitcoin/pull/31462#discussion_r1907622464) but other than that I'm happy with this step forward.

🤔 ryanofsky reviewed a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2537837742)
Code review 578654c686e394d08bac7c3bcddbfd90b46eaa62. Looks pretty good, but there does appear to be a possible overflow bug https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907214794 found by stickies that should be fixed

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907588881)
In commit "init: Simplify coinsdb cache calculation" (19316eccaf93776c49b1870f8aa9a5fe57ec5f33)

Didn't notice this first time I reviewed this, but it would be good if commit subject included "refactor" and make it obvious this is not changing behavior. This simplification is only changing the way the calculation is performed, not changing the result of the calculation.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907647674)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907214794

Agree with stickies overflow should be fixed. I think an easy way would just be to check if `db_cache > std::numeric_limits<size_t>::max() >> 20` though.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907618229)
In commit "kernel: Move kernel-specific cache size options to kernel" (41bc7164880ffd0782ff5882211c4fcadd656022)

This seems like it is overthinking things. I think this should just be:

```c++
Assert(mib >= 0);
Assert(mib <= std::numeric_limits<size_t>::max() >> 20);
return mib << 20;
```

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907604163)
To be pedantic, old calculation could only be useful if nMaxCoinsDBCache was *higher* not lower than 8 MiB. As long as it is 8MiB or lower this commit does not change behavior.

In #6102 the coins_db cache size was set to min(50% total cache size, 8MiB + 25% total cache size). But then in #8273 it was capped to nMaxCoinsDBCache, so as long as nMaxCoinsDBCache <= 8 MiB, this is just equivalent to min(50% total cache size, nMaxCoinsDBCache)

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907639861)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1907068544

> and adds a test case

I don't think I see a new test case added in the diff. Is it missing?

Another way to make this testable without adding interface complexity would just be to throw std::overflow_error

🤔 furszy reviewed a pull request: "wallet, desc spkm: Return SigningProvider only if we have the privkey"
(https://github.com/bitcoin/bitcoin/pull/31242#pullrequestreview-2537913150)
utACK f6a6d912059. Only reviewed the actual change in detail, not the test commit.

💬 furszy commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#discussion_r1907633404)
nit: `return keys.contains(keyid);`

💬 furszy commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#discussion_r1907650474)
for the sake of consistency across overloaded methods, `GetSigningProvider(script, include_private)` should behave in the same manner (the function that is just above this one).

💬 kehiy commented on pull request "doc: upgrade license to 2025.":
(https://github.com/bitcoin/bitcoin/pull/31611#issuecomment-2578428837)
@maflcko hi, can you review this one, please? the previous one was made on github, and i wasn't able to do the squash on github.

👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2537992108)
Code review ACK d925a9bd7a04f068ede227addbaef3025b9793b9. Lot of nice simplifications since last review: using references instead of pointers, getting rid of intermediate variables, using chainman.GetConsensus(), also improving release notes and test documentation and updating more RPCs to return the bits and target fields.

It would be really nice to remove the large test data file, so happy to rereview when the new test is ready.

👍 ryanofsky approved a pull request: "rpc: Remove deprecated dummy alias for listtransactions::label"
(https://github.com/bitcoin/bitcoin/pull/31413#pullrequestreview-2538004541)
Code review ACK fa8e0956c23789fb819ad1b31377711df091ec1b

👍 luke-jr approved a pull request: "doc: corrected lockunspent rpc quoting"
(https://github.com/bitcoin/bitcoin/pull/31275#pullrequestreview-2538131514)
Commit messages leave a bit to be desired, but the code and output looks correct

crACK 1f3f5c049b4080ecaf30604fd22d65aa0fc4af45

🤔 pablomartin4btc reviewed a pull request: "test: Remove --noshutdown flag, Tidy startup failures"
(https://github.com/bitcoin/bitcoin/pull/31620#pullrequestreview-2538211296)
cr ACK and tACK fae3bf6b870eb0f9cddd1adac82ba72890806ae3 (coming from #31462).

Adding `sys.stderr` was necessary for the CI checks (1st commit - fad441fba07877ea78ed6020fde11828307273a6). Redundant error messages were eliminated (e.g. in the proposed PR's [test](https://github.com/bitcoin/bitcoin/pull/31620#issuecomment-2577352102): "`Error: no RPC connection`" is shown only once instead of twice as in `master`).

Just for reference, `--noshutdown` was added in #6032 and `--pdbonfailure` in
...

👍 pinheadmz approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2538119452)
ACK 2fb833e69627a8f9b1c4b14b60d1df9903adf210

Reviewed each commit, built and ran unit and functional tests on arm/macos. I left a few questions and nits below.

Test coverage is great and I think this is the right approach to add a coinbase reserved weight option. Also it's a nice way to get those extra 4000 weight for default miners without putting the heavy-coinbase mining pools at risk



I also have one last lingering question which is: when does the miner actually construct their
...