💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1706029402)
in 29cac3e85b23f1264121cbd50e4a09755b0c8352
I think this commit may benefit from being two smaller commits, one that demonstrates the caching, and a second that demonstrates using it to skip the splitting computation?
I do not understand what it's doing, as-is.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1706029402)
in 29cac3e85b23f1264121cbd50e4a09755b0c8352
I think this commit may benefit from being two smaller commits, one that demonstrates the caching, and a second that demonstrates using it to skip the splitting computation?
I do not understand what it's doing, as-is.
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705789091)
I do realize the argument changes later, so maybe this just on the later commit
```Suggestion
queue.emplace_back(/*inc=*/SetInfo<SetType>{}, /*und=*/std::move(component), /*pot_feerate=*/FeeFrac{});
```
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705789091)
I do realize the argument changes later, so maybe this just on the later commit
```Suggestion
queue.emplace_back(/*inc=*/SetInfo<SetType>{}, /*und=*/std::move(component), /*pot_feerate=*/FeeFrac{});
```
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1707280073)
why 64?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1707280073)
why 64?
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1707294690)
why 4?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1707294690)
why 4?
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705747742)
I think this can be used with DepGraphFormatter during `Unser` with reversed `reordering`
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705747742)
I think this can be used with DepGraphFormatter during `Unser` with reversed `reordering`
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1704526813)
*non-empty* topological subsets?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1704526813)
*non-empty* topological subsets?
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705913641)
I would have appreciated slightly more pedantic comments demonstrating how the computation matches the comments :sweat:
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705913641)
I would have appreciated slightly more pedantic comments demonstrating how the computation matches the comments :sweat:
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705666485)
move of `m_todo` instantiation seems like a no-op?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1705666485)
move of `m_todo` instantiation seems like a no-op?
🤔 TheCharlatan reviewed a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2223960433)
lgtm , I hope we can finally get this project under way now.
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2223960433)
lgtm , I hope we can finally get this project under way now.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707213519)
Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707213519)
Just a question: Intuitively I was under the impression that only the node should be listening. In which scenario should the gui be able to listen as well? Do we need a bi-directional connection here for some callbacks?
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706885348)
If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don't exist, they will be created. Should that be documented here?
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706885348)
If a relative path is provided, the path of the data directory is appended. If the parent directories in the provided path don't exist, they will be created. Should that be documented here?
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253)
Nit: Could be declared `static`?
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706754253)
Nit: Could be declared `static`?
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706927164)
Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the "good" value.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1706927164)
Nit (personal style opinion): I feel like this (and the other functions in this file) would be a bit easier to read if the error conditions would return early. Then all this logic would not have to be indented and the final return of the function would carry the "good" value.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707289963)
Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707289963)
Is there a reason for manually resetting these here? I guess in the other test resetting is required, because otherwise the thread will never join.
💬 TheCharlatan commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707288885)
Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1707288885)
Since multiple binds are supported, I would suggest to add a test case for binding and connecting on different addresses in the same process.
💬 dergoegge commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273782709)
> Are you sure? I think may be confused with https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2258390205?
Iirc, I pinged Antoine for both PRs on irc.
I also don't think the stability needs to be a blocker.
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2273782709)
> Are you sure? I think may be confused with https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2258390205?
Iirc, I pinged Antoine for both PRs on irc.
I also don't think the stability needs to be a blocker.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707318080)
I don't think there is any difference, i just default to test against mainnet when possible. On the other hand it sometimes makes sense to default to regtest instead (for instance in the context of a fuzz target touching validation it would enable the codepaths for all soft forks). So, yeah, :shrug:.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707318080)
I don't think there is any difference, i just default to test against mainnet when possible. On the other hand it sometimes makes sense to default to regtest instead (for instance in the context of a fuzz target touching validation it would enable the codepaths for all soft forks). So, yeah, :shrug:.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707322786)
Actually i think i can get rid of this now, and just disable logging like i do like in #29158.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1707322786)
Actually i think i can get rid of this now, and just disable logging like i do like in #29158.
👍 Sjors approved a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2224313095)
tACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb
### Stalling fix
I tested (rebased on master) that without the second commit, when loading a testnet3 snapshot, and only having outbound peers, many peers keep downloading background sync blocks. A few newly connected peers do contribute to the tip.
With the second commit `synced_blocks` jumps to the snapshot height. Once the tip is reached (takes a while due to the loppocalypse and because the snapshot is a year old), and background sync ne
...
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2224313095)
tACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb
### Stalling fix
I tested (rebased on master) that without the second commit, when loading a testnet3 snapshot, and only having outbound peers, many peers keep downloading background sync blocks. A few newly connected peers do contribute to the tip.
With the second commit `synced_blocks` jumps to the snapshot height. Once the tip is reached (takes a while due to the loppocalypse and because the snapshot is a year old), and background sync ne
...
❤1
💬 Sjors commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1706829412)
7a885518d57c6eb818ebef5fd04a575f324ee8b2: although this scenario is worthy of `LogWarning`, we have to be careful to avoid a log spam attack. So for now `LogDebug` is fine.
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1706829412)
7a885518d57c6eb818ebef5fd04a575f324ee8b2: although this scenario is worthy of `LogWarning`, we have to be careful to avoid a log spam attack. So for now `LogDebug` is fine.