💬 ryanofsky commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1918877171)
In commit "build: depends makes libmultiprocess by default" (a77a88a7583e7f07626b55347cb6d3f31be8bffb)
It seems like this change is forcing multiprocess to be disabled for windows (ignoring the NO_MULTIPROCESS setting) instead of just defaulting it to off for windows. If that is really the intention here, I think it would be good to have a comment pointing that out, otherwise it's not clear why this code is explicitly mentioning linux and darwin.
I also wonder if a better alternative inste
...
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1918877171)
In commit "build: depends makes libmultiprocess by default" (a77a88a7583e7f07626b55347cb6d3f31be8bffb)
It seems like this change is forcing multiprocess to be disabled for windows (ignoring the NO_MULTIPROCESS setting) instead of just defaulting it to off for windows. If that is really the intention here, I think it would be good to have a comment pointing that out, otherwise it's not clear why this code is explicitly mentioning linux and darwin.
I also wonder if a better alternative inste
...
💬 mzumsande commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1917559601)
It seem a bit redundant to add the tx to more than one workset. Either we have all the parents now or we don't, so I don't see a point in attempting to validate it multiple times.
Assigning it to just one peer work set would run the risk of that peer disconnecting just after we assigned it to them, but that could be prevented by adding a last call to `ProcessOrphanTx()` in `FinalizeNode()` .
On the other hand, checking if we have all parents is probably cheap enough that it doesn't really ma
...
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1917559601)
It seem a bit redundant to add the tx to more than one workset. Either we have all the parents now or we don't, so I don't see a point in attempting to validate it multiple times.
Assigning it to just one peer work set would run the risk of that peer disconnecting just after we assigned it to them, but that could be prevented by adding a last call to `ProcessOrphanTx()` in `FinalizeNode()` .
On the other hand, checking if we have all parents is probably cheap enough that it doesn't really ma
...
💬 hebasto commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918905653)
If you imply that 3.17.0 is a CMake version, it is not. It’s `CMAKE_SYSTEM_VERSION`.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1918905653)
If you imply that 3.17.0 is a CMake version, it is not. It’s `CMAKE_SYSTEM_VERSION`.
👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2556780920)
Code review ACK cb305da72df501991a6700cd20be79dde4591184, moving coinbase key information from the test to the data readme file. I think this change makes both easier to understand now.
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2556780920)
Code review ACK cb305da72df501991a6700cd20be79dde4591184, moving coinbase key information from the test to the data readme file. I think this change makes both easier to understand now.
🚀 fanquake merged a pull request: "wallet, desc spkm: Return SigningProvider only if we have the privkey"
(https://github.com/bitcoin/bitcoin/pull/31242)
(https://github.com/bitcoin/bitcoin/pull/31242)
💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918971327)
56d5a6a495bfa5da540e9832443ce8f130c4c960: nit: `res` is not being used.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918971327)
56d5a6a495bfa5da540e9832443ce8f130c4c960: nit: `res` is not being used.
💬 theuni commented on pull request "init: Lock blocksdir in addition to datadir":
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2596381388)
This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.
(https://github.com/bitcoin/bitcoin/pull/31674#issuecomment-2596381388)
This caused an incompatibility with blocksdirs from previous versions of Core. Setting to draft whlie I play around with a fix.
📝 theuni converted_to_draft 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.
💬 theStack commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2596396995)
Concept ACK
Might be worth noting that this is currently only available on POSIX systems (i.e. not available on Windows, but on all other systems that we support AFAICT) in both the RPC help and a release note.
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2596396995)
Concept ACK
Might be worth noting that this is currently only available on POSIX systems (i.e. not available on Windows, but on all other systems that we support AFAICT) in both the RPC help and a release note.
💬 maflcko commented on pull request "doc: Update dependency installation for Debian/Ubuntu":
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596407548)
weak ACK 160c27ec078346fbf07f9b84dc10cef2b9809327
The code changes look fine, and the passing CI seems to indicate that pkgconf works even on Jammy and Bullseye. However, I haven't spend much time looking into the background here.
(https://github.com/bitcoin/bitcoin/pull/31621#issuecomment-2596407548)
weak ACK 160c27ec078346fbf07f9b84dc10cef2b9809327
The code changes look fine, and the passing CI seems to indicate that pkgconf works even on Jammy and Bullseye. However, I haven't spend much time looking into the background here.
💬 pablomartin4btc commented on pull request "Add new "address type" column to the "receiving tab" address book page":
(https://github.com/bitcoin-core/gui/pull/753#issuecomment-2596476999)
Rebased.
(https://github.com/bitcoin-core/gui/pull/753#issuecomment-2596476999)
Rebased.
💬 davidgumberg commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2596507213)
> Related to #31476.
🤦 I did not realize that's what #31593 was fixing.
> Assuming the CI is always configured to use `-DWERROR`, this seems like the more logical/generic solution for #31476 compared to #31669. Although one thing that could be improved is making the error more clear, as in the current output, it's hidden far back in the scrollback.
Done, by logging `configure_warnings` to the cmake-configure-log.
> > if we start using syntax features and behavior that are only ava
...
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2596507213)
> Related to #31476.
🤦 I did not realize that's what #31593 was fixing.
> Assuming the CI is always configured to use `-DWERROR`, this seems like the more logical/generic solution for #31476 compared to #31669. Although one thing that could be improved is making the error more clear, as in the current output, it's hidden far back in the scrollback.
Done, by logging `configure_warnings` to the cmake-configure-log.
> > if we start using syntax features and behavior that are only ava
...
📝 glozow opened a pull request: "[test] fix p2p_orphan_handling.py empty orphanage check"
(https://github.com/bitcoin/bitcoin/pull/31675)
Fix CI: https://cirrus-ci.com/task/4584274617171968?logs=ci#L2784
(https://github.com/bitcoin/bitcoin/pull/31675)
Fix CI: https://cirrus-ci.com/task/4584274617171968?logs=ci#L2784
💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919049643)
#31675
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1919049643)
#31675
🤔 BrandonOdiwuor reviewed a pull request: "rpc: add cpu_load to getpeerinfo"
(https://github.com/bitcoin/bitcoin/pull/31672#pullrequestreview-2557008931)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31672#pullrequestreview-2557008931)
Concept ACK
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1919054574)
I meant as `LINK_OPTIONS`. I'd rather have LDFLAGS forward to that unless there's a reason not to.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1919054574)
I meant as `LINK_OPTIONS`. I'd rather have LDFLAGS forward to that unless there's a reason not to.
💬 maflcko commented on pull request "[test] fix p2p_orphan_handling.py empty orphanage check":
(https://github.com/bitcoin/bitcoin/pull/31675#issuecomment-2596559337)
lgtm ACK 2e75ebb6169da08b04c4769555c4c84d6b5ca0ec
I guess this is a race from disconnecting the p2ps. So waiting seems the correct fix.
(https://github.com/bitcoin/bitcoin/pull/31675#issuecomment-2596559337)
lgtm ACK 2e75ebb6169da08b04c4769555c4c84d6b5ca0ec
I guess this is a race from disconnecting the p2ps. So waiting seems the correct fix.
👋 hebasto's pull request is ready for review: "build: Enhance Ccache performance across worktrees and build trees"
(https://github.com/bitcoin/bitcoin/pull/30861)
(https://github.com/bitcoin/bitcoin/pull/30861)
💬 glozow commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2596595387)
> > Good find! I think that `p2p_orphan_handling.py` has a similar [spot](https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/test/functional/p2p_orphan_handling.py#L431).
>
> Ah right, missed that one. If I interpret the `test_orphan_inherit_rejection` sub-test correctly, we want to send the INV with MSG_TX there to "announce again with potentially different witness", so the solution in this case to avoid ignoring the INV would be to add peer3 with wtxidrelay=Fal
...
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2596595387)
> > Good find! I think that `p2p_orphan_handling.py` has a similar [spot](https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/test/functional/p2p_orphan_handling.py#L431).
>
> Ah right, missed that one. If I interpret the `test_orphan_inherit_rejection` sub-test correctly, we want to send the INV with MSG_TX there to "announce again with potentially different witness", so the solution in this case to avoid ignoring the INV would be to add peer3 with wtxidrelay=Fal
...
💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610)
> What is the status of this?
Reworked. The PR description has been updated.
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2596595610)
> What is the status of this?
Reworked. The PR description has been updated.