🚀 fanquake merged a pull request: "doc: fix minor typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31673)
(https://github.com/bitcoin/bitcoin/pull/31673)
💬 mzumsande commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2596162834)
> My understanding is that a change like this would make the test fail on master, and pass with the patch in the pull request as it is now. But I haven't tried it, so I may be missing something?
I suspect that TheStack was looking for something that would prevent the underlying issue (sending wrong inv type) in general:
For example we could add an `assert` in `send_message` of `p2p.py` if we try to send an `Inv` of type `MSG_TX` and have `self.wtxidrelay` set (and vice versa). But it seems
...
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2596162834)
> My understanding is that a change like this would make the test fail on master, and pass with the patch in the pull request as it is now. But I haven't tried it, so I may be missing something?
I suspect that TheStack was looking for something that would prevent the underlying issue (sending wrong inv type) in general:
For example we could add an `assert` in `send_message` of `p2p.py` if we try to send an `Inv` of type `MSG_TX` and have `self.wtxidrelay` set (and vice versa). But it seems
...
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918849236)
I'm fine with leaving it out.
I also dropped the `COINBASE_OUTPUT_DESCRIPTOR` constant and moved the descriptor itself to the README.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918849236)
I'm fine with leaving it out.
I also dropped the `COINBASE_OUTPUT_DESCRIPTOR` constant and moved the descriptor itself to the README.
💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918868486)
I had same doubt. I understand that the generated address would be watch-only if we do not import as it's done in the test.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918868486)
I had same doubt. I understand that the generated address would be watch-only if we do not import as it's done in the test.
💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918877108)
0f0ce579fffe533534dc117c854ebef3f277f18e: nit: No need to set `require_checksum`. It's false by default.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918877108)
0f0ce579fffe533534dc117c854ebef3f277f18e: nit: No need to set `require_checksum`. It's false by default.
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918878124)
Why does `CMAKE_REQUIRED_LIBRARIES` get `LDFLAGS` rather than setting `LINK_OPTIONS`?
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918878124)
Why does `CMAKE_REQUIRED_LIBRARIES` get `LDFLAGS` rather than setting `LINK_OPTIONS`?
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918882693)
Ah, I see your comment here: https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917262536
I think a static lib would work fine as an option though? Or -l/path/to/libfoo.a otherwise?
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918882693)
Ah, I see your comment here: https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917262536
I think a static lib would work fine as an option though? Or -l/path/to/libfoo.a otherwise?
💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918886382)
> I think a static lib would work fine as an option though?
Correct. This is documented by CMake.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918886382)
> I think a static lib would work fine as an option though?
Correct. This is documented by CMake.
👍 ryanofsky approved a pull request: "Add multiprocess binaries to release build (except Windows)"
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2556701386)
Code review ACK 12357ee9482dafb021350965fb3c3ea718fa126c assuming CI passes since it is still running currently. I'll go ahead and update #31375 after this but I don't think it needs to be a dependency. It also seems good to bump libmultiprocess version in this PR I generally think it is it is good to batch changes and only bump where needed in cases like this.
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2556701386)
Code review ACK 12357ee9482dafb021350965fb3c3ea718fa126c assuming CI passes since it is still running currently. I'll go ahead and update #31375 after this but I don't think it needs to be a dependency. It also seems good to bump libmultiprocess version in this PR I generally think it is it is good to batch changes and only bump where needed in cases like this.
💬 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.