💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918760332)
The plot thickens - reference to lower version than minimum under *depends/*:
https://github.com/bitcoin/bitcoin/blob/df8bf657450d5383870d40790ea9f4fdb03c360d/depends/hosts/linux.mk#L44-L45
I agree with the author in https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760 that it's fine to limit the scope of the current PR.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918760332)
The plot thickens - reference to lower version than minimum under *depends/*:
https://github.com/bitcoin/bitcoin/blob/df8bf657450d5383870d40790ea9f4fdb03c360d/depends/hosts/linux.mk#L44-L45
I agree with the author in https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589465760 that it's fine to limit the scope of the current PR.
📝 adlai opened a pull request: "doc: fix minor typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31673)
In the unrelated PR #31621 the linter reported a few typos, that are fixed in this commit. I used the "doc" prefix as it only modifies comments, so none of the more significant prefixes seem appropriate.
(https://github.com/bitcoin/bitcoin/pull/31673)
In the unrelated PR #31621 the linter reported a few typos, that are fixed in this commit. I used the "doc" prefix as it only modifies comments, so none of the more significant prefixes seem appropriate.
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918779247)
Nah it's not a big deal i should probably find a way to run the linter stuff locally to prevent having to play ping pong like this.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918779247)
Nah it's not a big deal i should probably find a way to run the linter stuff locally to prevent having to play ping pong like this.
💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2596067990)
The example holds also for the template: Externally they are exactly the same, so they could at most affect the function body, which is 2 or 3 lines. I just don't see what type of issue the change is trying to prevent, or what benefit it brings to developers or users.
(For reference, the commit that drops the unused casts (https://github.com/bitcoin/bitcoin/commit/f23d6a572d62ff640a28718ff0771c3f0f87f365) has already been merged.)
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2596067990)
The example holds also for the template: Externally they are exactly the same, so they could at most affect the function body, which is 2 or 3 lines. I just don't see what type of issue the change is trying to prevent, or what benefit it brings to developers or users.
(For reference, the commit that drops the unused casts (https://github.com/bitcoin/bitcoin/commit/f23d6a572d62ff640a28718ff0771c3f0f87f365) has already been merged.)
💬 maflcko commented on pull request "doc: fix minor typos in comments":
(https://github.com/bitcoin/bitcoin/pull/31673#issuecomment-2596072482)
lgtm ACK b30cc71e853cbaaec66e7e23d7d1a32468079ee0
(https://github.com/bitcoin/bitcoin/pull/31673#issuecomment-2596072482)
lgtm ACK b30cc71e853cbaaec66e7e23d7d1a32468079ee0
💬 maflcko commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918808035)
> i should probably find a way to run the linter stuff locally
In theory it became a little bit easier after cmake, but it still requires a configure with `clang`. The you can possibly use the `clang-tidy-diff` approach explained in the dev notes. Though, for me it is rare enough to hit, that I am just playing the ping-pong. :sweat_smile:
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1918808035)
> i should probably find a way to run the linter stuff locally
In theory it became a little bit easier after cmake, but it still requires a configure with `clang`. The you can possibly use the `clang-tidy-diff` approach explained in the dev notes. Though, for me it is rare enough to hit, that I am just playing the ping-pong. :sweat_smile:
📝 theuni opened a pull request: "init: Lock blocksdir in addition to datadir"
(https://github.com/bitcoin/bitcoin/pull/31674)
This probably should've been included in #12653 when `-blocksdir` was introduced. Credit TheCharlatan for noticing that it's missing.
This guards against 2 processes running with separate datadirs but the same blocksdir. I didn't add `walletdir` as I assume sqlite has us covered there.
It's not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.
(https://github.com/bitcoin/bitcoin/pull/31674)
This probably should've been included in #12653 when `-blocksdir` was introduced. Credit TheCharlatan for noticing that it's missing.
This guards against 2 processes running with separate datadirs but the same blocksdir. I didn't add `walletdir` as I assume sqlite has us covered there.
It's not likely to happen currently, but may be more relevant in the future with applications using the kernel. Note that the kernel does not currently do any dir locking, but it should.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918811904)
Taken!
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918811904)
Taken!
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918812960)
fixed, even better I clarified that it also included the block header and txs count.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918812960)
fixed, even better I clarified that it also included the block header and txs count.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813151)
Done
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813151)
Done
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813433)
Done
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813433)
Done
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813734)
Done
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813734)
Done
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813936)
Taken, thanks
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813936)
Taken, thanks
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918814186)
Done
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918814186)
Done
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2596117348)
Forced pushed from ad1bc03245181b00a25ea0182373eddae1c151e1 to fd3103142de8bcb92b5db6a6501c4c66abc12abd see [diff](https://github.com/bitcoin/bitcoin/compare/ad1bc03245181b00a25ea0182373eddae1c151e1..fd3103142de8bcb92b5db6a6501c4c66abc12abd)
Changes
1. Addressed @Sjors comments
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2596117348)
Forced pushed from ad1bc03245181b00a25ea0182373eddae1c151e1 to fd3103142de8bcb92b5db6a6501c4c66abc12abd see [diff](https://github.com/bitcoin/bitcoin/compare/ad1bc03245181b00a25ea0182373eddae1c151e1..fd3103142de8bcb92b5db6a6501c4c66abc12abd)
Changes
1. Addressed @Sjors comments
💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918820905)
yes, I think it needs to be a `wait_until`
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918820905)
yes, I think it needs to be a `wait_until`
👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2556604025)
Code review ACK 39eabfa433e0bb9d85c67c6f9ce74c1570a21417. Since last review just simplified the test and clarified documentation a bit (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2556604025)
Code review ACK 39eabfa433e0bb9d85c67c6f9ce74c1570a21417. Since last review just simplified the test and clarified documentation a bit (thanks!)
💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918817921)
In commit "test: check difficulty adjustment using alternate mainnet" (90b6c189d9b18203e119a5f3478f7b0da7d71392)
Note: this seems ok, but I guess it could be simplified a little since COINBASE_OUTPUT_DESCRIPTOR is unused, and no spending is done anymore.
In case you do want to bring back the spending test that seems fine if it's explained a little. I wasn't necessarily asking to remove it before, but was just confused by why it was there and a little overwhelmed with the details chain gene
...
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918817921)
In commit "test: check difficulty adjustment using alternate mainnet" (90b6c189d9b18203e119a5f3478f7b0da7d71392)
Note: this seems ok, but I guess it could be simplified a little since COINBASE_OUTPUT_DESCRIPTOR is unused, and no spending is done anymore.
In case you do want to bring back the spending test that seems fine if it's explained a little. I wasn't necessarily asking to remove it before, but was just confused by why it was there and a little overwhelmed with the details chain gene
...
👋 Sjors's pull request is ready for review: "Add multiprocess binaries to release build (except Windows)"
(https://github.com/bitcoin/bitcoin/pull/30975)
(https://github.com/bitcoin/bitcoin/pull/30975)
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2596134377)
The first commit now bumps the multiprocess dependency. @ryanofsky feel free to extract into its own PR if you want to land it faster.
I don't it's necessary to wait for #31375 and #31161 so I've marked this ready for review. But if folks disagree, I can make it draft again.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2596134377)
The first commit now bumps the multiprocess dependency. @ryanofsky feel free to extract into its own PR if you want to land it faster.
I don't it's necessary to wait for #31375 and #31161 so I've marked this ready for review. But if folks disagree, I can make it draft again.