💬 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` ?
💬 luke-jr commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2578616268)
>i'm not aware of a straightforward way to "log network traffic of this process and subproceses only".
Not quite that, but iptables has the ability to match and log uid
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2578616268)
>i'm not aware of a straightforward way to "log network traffic of this process and subproceses only".
Not quite that, but iptables has the ability to match and log uid
🤔 furszy reviewed a pull request: "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors"
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2538306680)
Code review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2538306680)
Code review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
💬 furszy commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1907872493)
tiny nit: the `validation` category is a bit odd in the wallet scan context or GUI updates.
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1907872493)
tiny nit: the `validation` category is a bit odd in the wallet scan context or GUI updates.
💬 luke-jr commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#issuecomment-2578726192)
> this isn't being backported.
Why not?
(https://github.com/bitcoin/bitcoin/pull/31374#issuecomment-2578726192)
> this isn't being backported.
Why not?
💬 luke-jr commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2578764484)
To have the intended effect, "mintime" needs to be adjusted too, like in #31600
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2578764484)
To have the intended effect, "mintime" needs to be adjusted too, like in #31600
💬 achow101 commented on pull request "wallet: fix crash during watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/31374#issuecomment-2578773379)
> > this isn't being backported.
>
> Why not?
It was unclean, and making it a clean backport seemed to be a somewhat large change.
(https://github.com/bitcoin/bitcoin/pull/31374#issuecomment-2578773379)
> > this isn't being backported.
>
> Why not?
It was unclean, and making it a clean backport seemed to be a somewhat large change.
💬 luke-jr commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2578782712)
> we are enforcing this rules within the descriptors parsing logic.
That sounds dumb. Why are we?
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2578782712)
> we are enforcing this rules within the descriptors parsing logic.
That sounds dumb. Why are we?
💬 pinheadmz commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2578808220)
@luke-jr can you rephrase your comment in a friendlier way please
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2578808220)
@luke-jr can you rephrase your comment in a friendlier way please