💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955125902)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953057497
Thanks! Applied
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955125902)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953057497
Thanks! Applied
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955101327)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952943101
(note: renamed `search_system_path` to `fallback_os_search` in the latest push)
> I don't get it why `search_system_path` is used as `allow_notfound`. Those are two different things.
Exactly, they are two different things.
If `search_system_path` is true, then we want to allow the operating system to search the system path. In order for that to happen we need our own searches to not throw a fatal error, therefor
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955101327)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952943101
(note: renamed `search_system_path` to `fallback_os_search` in the latest push)
> I don't get it why `search_system_path` is used as `allow_notfound`. Those are two different things.
Exactly, they are two different things.
If `search_system_path` is true, then we want to allow the operating system to search the system path. In order for that to happen we need our own searches to not throw a fatal error, therefor
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955130244)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953105287
> The `dtrace` example would print all exec calls on the system, not just the ones by the given program.
Wow previous behavior would not be great, applied your suggestion.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955130244)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953105287
> The `dtrace` example would print all exec calls on the system, not just the ones by the given program.
Wow previous behavior would not be great, applied your suggestion.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955147970)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953127351
> intermediate state where the code is broken on Windows
Thanks, restructured so this is no longer the case. I didn't think anyone would care if an intermediate non-merge commit failed to compile on windows and I thought code would be easier to understand looking at the unix version first. But in retrospect a better way to separate unix and windows code is just to move the windows code to another file, so have now don
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955147970)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953127351
> intermediate state where the code is broken on Windows
Thanks, restructured so this is no longer the case. I didn't think anyone would care if an intermediate non-merge commit failed to compile on windows and I thought code would be easier to understand looking at the unix version first. But in retrospect a better way to separate unix and windows code is just to move the windows code to another file, so have now don
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955144011)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953144139
> I wonder if this can be made dumb stupid and reduce it to just call `execvp()`. That is, reduce `ExecCommand()` to just 1 - 5 lines of code and deal with the consequences of it...
It's not really clear what you could be suggesting specifically.
The only thing `ExecCommand()` is doing is trying to execute a few different versions of the provided command line with some prefixes added. It is basically just emulating
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955144011)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953144139
> I wonder if this can be made dumb stupid and reduce it to just call `execvp()`. That is, reduce `ExecCommand()` to just 1 - 5 lines of code and deal with the consequences of it...
It's not really clear what you could be suggesting specifically.
The only thing `ExecCommand()` is doing is trying to execute a few different versions of the provided command line with some prefixes added. It is basically just emulating
...
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955238525)
I don't think this is the place for it, as it amounts to testing of `LinearizationChunking`, not `TxGraph`. The point of the code here is just to get the chunks defined by `m_linearization` out, so we can test them. If `LinearizationChunking` works correctly (see `clusterlin_linearization_chunking` for that), then the chunks that come out will correspond to consecutive transactions from the linearization.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955238525)
I don't think this is the place for it, as it amounts to testing of `LinearizationChunking`, not `TxGraph`. The point of the code here is just to get the chunks defined by `m_linearization` out, so we can test them. If `LinearizationChunking` works correctly (see `clusterlin_linearization_chunking` for that), then the chunks that come out will correspond to consecutive transactions from the linearization.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955241097)
Will address on the next push.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955241097)
Will address on the next push.
💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1955247357)
That's backward. We need to go higher to speed up IBD. Lower slows it down.
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1955247357)
That's backward. We need to go higher to speed up IBD. Lower slows it down.
💬 hebasto commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657761654)
> @stickies-v Thanks for pointing that out, that was accidental! I actually didn't notice the difference. I think we should fix it so that they're aligned instead of having a single outlier.
>
> ~Though.. would it make sense to rename the target to `libbitcoinkernel` instead of changing the component to `bitcoinkernel`? I think the former would be more obvious?~
>
> Edit: nm, can't do that as CMake will rename the resulting file to liblibbitcoinkernel.so.
But we can:
```cmake
add_cust
...
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657761654)
> @stickies-v Thanks for pointing that out, that was accidental! I actually didn't notice the difference. I think we should fix it so that they're aligned instead of having a single outlier.
>
> ~Though.. would it make sense to rename the target to `libbitcoinkernel` instead of changing the component to `bitcoinkernel`? I think the former would be more obvious?~
>
> Edit: nm, can't do that as CMake will rename the resulting file to liblibbitcoinkernel.so.
But we can:
```cmake
add_cust
...
💬 furszy commented on pull request "wallet: fix rescanning inconsistency":
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2657775368)
After a few conversations with mzumzande, it seems to me that we could split the "scan blocks beyond the last processed block" issue into two scenarios:
(1) when the wallet mutex is locked for the entire time the rescanning process is executed. And (2) when the wallet mutex is not locked for the entire rescanning time.
(1) Is #31474 case:
Given that we set the "last processed block" to the chain tip just before calling the initial rescan. I do think this process expects to stop at the "l
...
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2657775368)
After a few conversations with mzumzande, it seems to me that we could split the "scan blocks beyond the last processed block" issue into two scenarios:
(1) when the wallet mutex is locked for the entire time the rescanning process is executed. And (2) when the wallet mutex is not locked for the entire rescanning time.
(1) Is #31474 case:
Given that we set the "last processed block" to the chain tip just before calling the initial rescan. I do think this process expects to stop at the "l
...
💬 davidgumberg commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1955257968)
Maybe better expressed as: Is trying to both override the compiler launcher and make configure_warnings errors a use case that should be supported? If so, making this message a `configure_warning` is incompatible with #31665 since that makes `configure_warnings` fatal errors if `WCONFIGURE_ERROR=ON`.
If it is not a use case that should be supported, then making this message a configure_warning might be good together with #31665, since it would cause an error in CI if ccache ever got disabled
...
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1955257968)
Maybe better expressed as: Is trying to both override the compiler launcher and make configure_warnings errors a use case that should be supported? If so, making this message a `configure_warning` is incompatible with #31665 since that makes `configure_warnings` fatal errors if `WCONFIGURE_ERROR=ON`.
If it is not a use case that should be supported, then making this message a configure_warning might be good together with #31665, since it would cause an error in CI if ccache ever got disabled
...
💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1955268028)
I meant that if we could avoid writing blocks (and especially undos, which likely don't serve any purpose during IBD as far as I can tell), we could speed up IBD measurably for pruned nodes.
If the reason for writing blocks is to avoid flushing to LevelDB, there may be better ways (as mentioned, https://github.com/bitcoin/bitcoin/pull/31645 + sorting speeds up dumping from e.g. 30 minutes to 7).
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1955268028)
I meant that if we could avoid writing blocks (and especially undos, which likely don't serve any purpose during IBD as far as I can tell), we could speed up IBD measurably for pruned nodes.
If the reason for writing blocks is to avoid flushing to LevelDB, there may be better ways (as mentioned, https://github.com/bitcoin/bitcoin/pull/31645 + sorting speeds up dumping from e.g. 30 minutes to 7).
💬 sipa commented on pull request "RFC: Generated headers with ""_hex user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31861#issuecomment-2657797259)
Eh, needing `-ftemplate-depth` and `-fconstexpr-steps` sounds pretty terrible. Thank you for investigating, though.
(https://github.com/bitcoin/bitcoin/pull/31861#issuecomment-2657797259)
Eh, needing `-ftemplate-depth` and `-fconstexpr-steps` sounds pretty terrible. Thank you for investigating, though.
💬 laanwj commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2657812796)
> It's unclear to me under what scenario the call might fail
The [ARM documentation](https://developer.arm.com/documentation/107997/0000/AArch64-registers/AArch64-Generic-System-Control-registers-summary/RNDRRS--Reseeded-Random-Number) isn't very specific on that:
> If the hardware returns a genuine random number, PSTATE.NZCV is set to 0b0000.
>
> If the instruction cannot return a genuine random number in a reasonable period of time, PSTATE.NZCV is set to 0b0100 and the data value retur
...
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2657812796)
> It's unclear to me under what scenario the call might fail
The [ARM documentation](https://developer.arm.com/documentation/107997/0000/AArch64-registers/AArch64-Generic-System-Control-registers-summary/RNDRRS--Reseeded-Random-Number) isn't very specific on that:
> If the hardware returns a genuine random number, PSTATE.NZCV is set to 0b0000.
>
> If the instruction cannot return a genuine random number in a reasonable period of time, PSTATE.NZCV is set to 0b0100 and the data value retur
...
✅ fjahr closed a pull request: "RFC: Generated headers with ""_hex user-defined literal"
(https://github.com/bitcoin/bitcoin/pull/31861)
(https://github.com/bitcoin/bitcoin/pull/31861)
💬 fjahr commented on pull request "RFC: Generated headers with ""_hex user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31861#issuecomment-2657816972)
Alright, closing ✅
(https://github.com/bitcoin/bitcoin/pull/31861#issuecomment-2657816972)
Alright, closing ✅
👍 hebasto approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2616350801)
ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d.
I have a slight preference for the approach mentioned [above](https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657761654).
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2616350801)
ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d.
I have a slight preference for the approach mentioned [above](https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657761654).
💬 laanwj commented on issue "GetRandBytes() Hangs on Samsung Galaxy S25 and OnePlus 13":
(https://github.com/bitcoin/bitcoin/issues/31817#issuecomment-2657838784)
> Build with: Android NDK 25.1.8937393 & 27.2.12479018
We've had some problems building on android (including NDK 25, see #29360), and are curious how you do the builds for that platform, for testing, can you list more specific build instructions please?
(https://github.com/bitcoin/bitcoin/issues/31817#issuecomment-2657838784)
> Build with: Android NDK 25.1.8937393 & 27.2.12479018
We've had some problems building on android (including NDK 25, see #29360), and are curious how you do the builds for that platform, for testing, can you list more specific build instructions please?
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955149722)
In "txgraph: (feature) make max cluster count configurable and oversize state" 370c419c330ca73d9ea44a7d48506a5c203e9c17
This is a duplicate I think
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955149722)
In "txgraph: (feature) make max cluster count configurable and oversize state" 370c419c330ca73d9ea44a7d48506a5c203e9c17
This is a duplicate I think
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955245898)
In "txgraph: (optimization) delay chunking while sub-acceptable" 30d7c8ce7f34e977238d1454dd032196cbfd936b
nit: we can have truthy/false method for this so that we can just call it.
we have this conditional statement in quite a few places
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955245898)
In "txgraph: (optimization) delay chunking while sub-acceptable" 30d7c8ce7f34e977238d1454dd032196cbfd936b
nit: we can have truthy/false method for this so that we can just call it.
we have this conditional statement in quite a few places