💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955205625)
Opened #31861 so it can be discussed there :)
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955205625)
Opened #31861 so it can be discussed there :)
💬 achow101 commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2657705795)
I am able to replicate @hodlinator's errors with cross built binaries.
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2657705795)
I am able to replicate @hodlinator's errors with cross built binaries.
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657714870)
> During designing the BIP, we decided that LIMITED NODES should always cut off at HEAD-288 to avoid getting fingerprinted by leaking the prune depth.
Maybe the BIP and documentation here should clarify that limited nodes are defined by both the limited bit being set and by the node network bit not being set, as described in https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2654830209.
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657714870)
> During designing the BIP, we decided that LIMITED NODES should always cut off at HEAD-288 to avoid getting fingerprinted by leaking the prune depth.
Maybe the BIP and documentation here should clarify that limited nodes are defined by both the limited bit being set and by the node network bit not being set, as described in https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2654830209.
💬 theuni commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657722669)
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657722669)
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2657724719)
> > Is this failure introduced by this PR, or does it occur on the master branch as well?
>
> I think the _way_ cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.
Right.
Even with an additional commit that fixes another [check](https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2651918799) in this test, some of the subsequent checks reveal their flaws. Fixing these issues will require removing non-portable behaviour in the wallet code. I'
...
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2657724719)
> > Is this failure introduced by this PR, or does it occur on the master branch as well?
>
> I think the _way_ cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.
Right.
Even with an additional commit that fixes another [check](https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2651918799) in this test, some of the subsequent checks reveal their flaws. Fixing these issues will require removing non-portable behaviour in the wallet code. I'
...
📝 hebasto converted_to_draft a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410)
Fixes https://github.com/bitcoin/bitcoin/issues/31409.
The first commit prevents possible false positives by ensuring that each condition potentially causing the "Error scanning" log message is tested separately.
The second commit disables a problematic check on Windows.
(https://github.com/bitcoin/bitcoin/pull/31410)
Fixes https://github.com/bitcoin/bitcoin/issues/31409.
The first commit prevents possible false positives by ensuring that each condition potentially causing the "Error scanning" log message is tested separately.
The second commit disables a problematic check on Windows.
💬 hebasto commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657729422)
> Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
What CMake version?
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657729422)
> Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.
What CMake version?
🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2616014706)
Thanks for the reviews!
Rebased 0e4ee158cadc3eb8f6af1b33440ae9a95fe19487 -> edb8a72226b362ee60e4184fd8f56d1e588ce5b6 ([`pr/wrap.18`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.18) -> [`pr/wrap.19`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.18-rebase..pr/wrap.19)) due to conflict with #31818
---
re: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2612338457
> Moving the python chan
...
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2616014706)
Thanks for the reviews!
Rebased 0e4ee158cadc3eb8f6af1b33440ae9a95fe19487 -> edb8a72226b362ee60e4184fd8f56d1e588ce5b6 ([`pr/wrap.18`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.18) -> [`pr/wrap.19`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.18-rebase..pr/wrap.19)) due to conflict with #31818
---
re: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2612338457
> Moving the python chan
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955118691)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952926320
Thanks, applied suggestion
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955118691)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952926320
Thanks, applied suggestion
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955114378)
https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952940524
> This is equivalent to [...] because `try_exec()` would only return if `search_system_path` is `true`
IMO, this code should work correctly even if `try_exec()` returns true when it succeeds. There's no reason a future implementation of `try_exec()` might not return true when it succeeds or that someone reading this code should automatically know try_exec() doesn't just act like a normal function and return true when it s
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955114378)
https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952940524
> This is equivalent to [...] because `try_exec()` would only return if `search_system_path` is `true`
IMO, this code should work correctly even if `try_exec()` returns true when it succeeds. There's no reason a future implementation of `try_exec()` might not return true when it succeeds or that someone reading this code should automatically know try_exec() doesn't just act like a normal function and return true when it s
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955091370)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952885840
(note: renamed this variable to `fallback_os_search` in the latest push)
> This means that the variable search_system_path will be wrongly set to true in this case.
It will be set to true, but I don't think it will be "wrongly" set to true. This is a heuristic deciding whether or not to let operating system look for executables if we can't find them ourselves. It's very possible different behaviors may be more or l
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955091370)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952885840
(note: renamed this variable to `fallback_os_search` in the latest push)
> This means that the variable search_system_path will be wrongly set to true in this case.
It will be set to true, but I don't think it will be "wrongly" set to true. This is a heuristic deciding whether or not to let operating system look for executables if we can't find them ourselves. It's very possible different behaviors may be more or l
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955128191)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953063825
Thanks! Applied
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955128191)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953063825
Thanks! Applied
💬 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.