💬 davidgumberg commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2655174694)
Tested crACK https://github.com/bitcoin/bitcoin/pull/31014/commits/851085a7f2761584402080b1767328d86cde4d94
Nice refactor in reusing `FromSockAddr()` and probably makes local address discovery on Windows less brittle.
Ran this branch and master on a local Windows 10 22H2 machine with multiple network adapters, both `Discover()`'ed the same IPv6 addresses, which matched up with what `ifconfig` reported as the machine's addresses. The device does not have any IPv4 addresses which pass `CNetA
...
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2655174694)
Tested crACK https://github.com/bitcoin/bitcoin/pull/31014/commits/851085a7f2761584402080b1767328d86cde4d94
Nice refactor in reusing `FromSockAddr()` and probably makes local address discovery on Windows less brittle.
Ran this branch and master on a local Windows 10 22H2 machine with multiple network adapters, both `Discover()`'ed the same IPv6 addresses, which matched up with what `ifconfig` reported as the machine's addresses. The device does not have any IPv4 addresses which pass `CNetA
...
💬 davidgumberg commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1953657923)
As-is this makes callers that use this helper have more control over the blocks that they're testing.
It shouldn't have a terrible impact, but excluding the transaction that creates the first set of `outputs` of the size and arrangement that the caller wants to test makes these benchmark scenarios more 'ideal', since it avoids testing validation of an input whose spending conditions the caller doesn't have any control over, since `TestChain100Setup` gets to decide the coinbase outputs.
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1953657923)
As-is this makes callers that use this helper have more control over the blocks that they're testing.
It shouldn't have a terrible impact, but excluding the transaction that creates the first set of `outputs` of the size and arrangement that the caller wants to test makes these benchmark scenarios more 'ideal', since it avoids testing validation of an input whose spending conditions the caller doesn't have any control over, since `TestChain100Setup` gets to decide the coinbase outputs.
💬 davidgumberg commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1953663478)
The reason to separate the first tx is because it goes into a block of its own. https://github.com/bitcoin/bitcoin/pull/31689/files#r1953657923
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1953663478)
The reason to separate the first tx is because it goes into a block of its own. https://github.com/bitcoin/bitcoin/pull/31689/files#r1953657923
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953783352)
Well `choices` counts the actual number of choices, consisting of:
* All transactions in sim.simmap (`tx_count`)
* All transactions in sim.removed (`sim.removed.size()`)
* The empty Ref (`1`).
When picking one of them, we want one in the range from 0 up to and including `choices - 1`.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953783352)
Well `choices` counts the actual number of choices, consisting of:
* All transactions in sim.simmap (`tx_count`)
* All transactions in sim.removed (`sim.removed.size()`)
* The empty Ref (`1`).
When picking one of them, we want one in the range from 0 up to and including `choices - 1`.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953785604)
It doesn't matter; `TxGraph` ignores self-dependencies anyway (because `DepGraph` does), so having it doesn't hurt, and adds testing for that case.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953785604)
It doesn't matter; `TxGraph` ignores self-dependencies anyway (because `DepGraph` does), so having it doesn't hurt, and adds testing for that case.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953788209)
It's not clear to me what you're asking for. I would describe the test as "big simulation test, which performs a number of operations on a real TxGraph, and on a simpler reimplementation, and in the end compares the two". Something like that? Which `cluster_linearize` fuzz test are you referring to?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953788209)
It's not clear to me what you're asking for. I would describe the test as "big simulation test, which performs a number of operations on a real TxGraph, and on a simpler reimplementation, and in the end compares the two". Something like that? Which `cluster_linearize` fuzz test are you referring to?
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953790028)
I'm not sure what you mean here.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953790028)
I'm not sure what you mean here.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953791082)
Agreed, fixed.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953791082)
Agreed, fixed.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953791264)
That was outdated. Fixed.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1953791264)
That was outdated. Fixed.
💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953815957)
Passing something like `search_system_path=true` to `try_exec()` is misleading because `try_exec()` uses `execvp(3)` which will search the system path but not based on the `search_system_path=true` argument but based on whether there is `/` in the executable: https://linux.die.net/man/3/execvp. Maybe rename `search_system_path` to `allow_notfound`?
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953815957)
Passing something like `search_system_path=true` to `try_exec()` is misleading because `try_exec()` uses `execvp(3)` which will search the system path but not based on the `search_system_path=true` argument but based on whether there is `/` in the executable: https://linux.die.net/man/3/execvp. Maybe rename `search_system_path` to `allow_notfound`?
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953823282)
Before it was like what you suggest, then the `check_tip_changed()` lambda was introduced because that code had to be called in two places, then, following suggestions, it was changed to be called in just one place. I agree it would be better to remove it now, given that it is called in just one place.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953823282)
Before it was like what you suggest, then the `check_tip_changed()` lambda was introduced because that code had to be called in two places, then, following suggestions, it was changed to be called in just one place. I agree it would be better to remove it now, given that it is called in just one place.
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953825128)
> Assume(tip_block) ... if tip_block is null the right thing to do is just wait ...
Hmm, if `tip_block` is null then `Assume()` will not wait but stop the program?
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1953825128)
> Assume(tip_block) ... if tip_block is null the right thing to do is just wait ...
Hmm, if `tip_block` is null then `Assume()` will not wait but stop the program?
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1953840453)
> if timeout is exceeded while waiting for a new tip, this returns information about the current tip
True. What I meant was that if there is no tip within the timeout. The function does `wait_for(... tip_hash && tip_hash != current_tip ...)` - `tip_hash` to be set and to be different than the current. I meant that it could remain unset while the timeout passes. I see how the comment I suggested is misleading. What about:
```cpp
@retval std::nullopt if the given `timeout` passes before the
...
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1953840453)
> if timeout is exceeded while waiting for a new tip, this returns information about the current tip
True. What I meant was that if there is no tip within the timeout. The function does `wait_for(... tip_hash && tip_hash != current_tip ...)` - `tip_hash` to be set and to be different than the current. I meant that it could remain unset while the timeout passes. I see how the comment I suggested is misleading. What about:
```cpp
@retval std::nullopt if the given `timeout` passes before the
...
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1953850030)
I mean to avoid `CHECK_NONFATAL()` if the tip is null and use `JSONRPCError` instead.
If the tip is null at the start, then wait for it to be set, if not set within the timeout, then `JSONRPCError`.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1953850030)
I mean to avoid `CHECK_NONFATAL()` if the tip is null and use `JSONRPCError` instead.
If the tip is null at the start, then wait for it to be set, if not set within the timeout, then `JSONRPCError`.
💬 davidgumberg commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2655685417)
ACK https://github.com/bitcoin/bitcoin/pull/31689/commits/1c6b886465df0f00549e7d10c3bfefd27be7f1c2
I like that the benchmarks added here for `ConnectBlock` are idealized scenarios that map to specific paths we want to test the performance of, namely Schnorr signature validation and ECDSA signature validation. Measuring performance in realistic scenarios is valuable for establishing ground truth, but it seems to me that microbenchmarks like these are most useful when their focus is narrow, sin
...
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2655685417)
ACK https://github.com/bitcoin/bitcoin/pull/31689/commits/1c6b886465df0f00549e7d10c3bfefd27be7f1c2
I like that the benchmarks added here for `ConnectBlock` are idealized scenarios that map to specific paths we want to test the performance of, namely Schnorr signature validation and ECDSA signature validation. Measuring performance in realistic scenarios is valuable for establishing ground truth, but it seems to me that microbenchmarks like these are most useful when their focus is narrow, sin
...
💬 maflcko commented on issue "Memory-only wallet (for faucets)":
(https://github.com/bitcoin/bitcoin/issues/31816#issuecomment-2655746412)
> Before I was doing also `scantxoutset` without any wallet. It actually provided me with unspent outputs that were confirmed in blocks and then I could just `signrawtransactionwithkey`. But using a wallet has many advantages
This would have been my suggestion as a workaround. Is there something that would be missing? I haven't tried, but `removeprunedfunds` could be another alternative to reduce the wallet file size?
(https://github.com/bitcoin/bitcoin/issues/31816#issuecomment-2655746412)
> Before I was doing also `scantxoutset` without any wallet. It actually provided me with unspent outputs that were confirmed in blocks and then I could just `signrawtransactionwithkey`. But using a wallet has many advantages
This would have been my suggestion as a workaround. Is there something that would be missing? I haven't tried, but `removeprunedfunds` could be another alternative to reduce the wallet file size?
👍 TheCharlatan approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2614109983)
ACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2614109983)
ACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90
👍 TheCharlatan approved a pull request: "guix: use GCC 13 to build releases"
(https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2614124544)
Re-ACK 0c1b29a05777
(https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2614124544)
Re-ACK 0c1b29a05777
💬 maflcko commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2655798240)
> @maflcko Mind taking a look at the first commit here to see if you'd prefer a different approach?
The first commit lgtm. Anything should be fine in the fuzz ci task as long as it compiles and runs the fuzz executable.
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2655798240)
> @maflcko Mind taking a look at the first commit here to see if you'd prefer a different approach?
The first commit lgtm. Anything should be fine in the fuzz ci task as long as it compiles and runs the fuzz executable.
💬 hodlinator commented on pull request "qa debug: Add --debugnode/-waitfordebugger [DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2655806242)
- Added CMake option `WAIT_FOR_DEBUGGER`.
- Changed the Python logic from debugging specific nodes, to debugging specific node runs/executions, which is more specific.
- Use `prctl()` on Linux to allow any process to attach to `bitcoind`. This removes the necessity to disable `kernel.yama.ptrace_scope` on some systems, which may be seen as decreasing security, but is compiled out unless `WAIT_FOR_DEBUGGER=ON`.
- (Confirmed PR works on Windows).
- (Rebased).
(https://github.com/bitcoin/bitcoin/pull/31723#issuecomment-2655806242)
- Added CMake option `WAIT_FOR_DEBUGGER`.
- Changed the Python logic from debugging specific nodes, to debugging specific node runs/executions, which is more specific.
- Use `prctl()` on Linux to allow any process to attach to `bitcoind`. This removes the necessity to disable `kernel.yama.ptrace_scope` on some systems, which may be seen as decreasing security, but is compiled out unless `WAIT_FOR_DEBUGGER=ON`.
- (Confirmed PR works on Windows).
- (Rebased).