🤔 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.
(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
(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.
(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.
(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;
```
(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)
(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
(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.
(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);`
(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).
(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.
(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.
(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
(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
(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
...
(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
...
(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
...
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907817185)
2fb833e69627a8f9b1c4b14b60d1df9903adf210
You might want to mention the old default of `3996000` here. I was staring at `blockmaxweight=4000000` on the following line for a minute trying to figure out what actually changed.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907817185)
2fb833e69627a8f9b1c4b14b60d1df9903adf210
You might want to mention the old default of `3996000` here. I was staring at `blockmaxweight=4000000` on the following line for a minute trying to figure out what actually changed.
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907760169)
b55bb95d8d80175f30d2d4da5c6cfd72e5d8eedd
nit: could be `LARGE_TXS_COUNT + 1` to make it clear the difference from previous template
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907760169)
b55bb95d8d80175f30d2d4da5c6cfd72e5d8eedd
nit: could be `LARGE_TXS_COUNT + 1` to make it clear the difference from previous template
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907807887)
a45af6af4e7a59cd14ef6e16b8e5e180fa9d564d
Question about this phrasing: what is "maximum" about it? If a user sets `-maxcoinbaseweight=1000` then 1000 weight will always be reserved in block templates, and if the actual coinbase ends up being 50 weight instead of 1000 that extra weight will just be unused and the final block will be smaller than maximum size.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907807887)
a45af6af4e7a59cd14ef6e16b8e5e180fa9d564d
Question about this phrasing: what is "maximum" about it? If a user sets `-maxcoinbaseweight=1000` then 1000 weight will always be reserved in block templates, and if the actual coinbase ends up being 50 weight instead of 1000 that extra weight will just be unused and the final block will be smaller than maximum size.
💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907812521)
a45af6af4e7a59cd14ef6e16b8e5e180fa9d564d
Should this test against `args.GetIntArg("-blockmaxweight", MAX_BLOCK_WEIGHT)` ? Or, what happens if a user sets `-blockmaxweight=1000 -maxcoinbaseweight=4000` ?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1907812521)
a45af6af4e7a59cd14ef6e16b8e5e180fa9d564d
Should this test against `args.GetIntArg("-blockmaxweight", MAX_BLOCK_WEIGHT)` ? Or, what happens if a user sets `-blockmaxweight=1000 -maxcoinbaseweight=4000` ?