💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286406511)
nit, I think this was wrong in the original test as well, should be:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders(std::span{first_chain}.last(first_chain.size() - 1), false),
```
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286406511)
nit, I think this was wrong in the original test as well, should be:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders(std::span{first_chain}.last(first_chain.size() - 1), false),
```
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286407697)
nit: I think this comment was incorrectly placed in the original, it is the `ProcessNextHeaders` above that needs `/*full_headers_message=*/true`
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286407697)
nit: I think this comment was incorrectly placed in the original, it is the `ProcessNextHeaders` above that needs `/*full_headers_message=*/true`
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286413175)
nit:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders({first_chain.begin() + REDOWNLOAD_BUFFER_SIZE + 1, first_chain.end()}, false),
```
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286413175)
nit:
```suggestion
CHECK_RESULT(hss.ProcessNextHeaders({first_chain.begin() + REDOWNLOAD_BUFFER_SIZE + 1, first_chain.end()}, false),
```
💬 achow101 commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2286418712)
Yes, I believe it is obsolete. Removed.
(https://github.com/bitcoin/bitcoin/pull/32523#discussion_r2286418712)
Yes, I believe it is obsolete. Removed.
💬 TheBlueMatt commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202364764)
> I don't see how it's required to use a fork of Core, unless you insist on running pre-built binaries only. This may be somewhat naive, but it doesn't seem that difficult to encourage users that want SV2 to use v30 with IPC enabled, and then open up issues as bugs turn up
- @marcofleon
Yes, this is somewhat naive. We've discussed this more than a few times and the reality is that people just don't build from source. We've been talking to miners for a long time and telling them "just take Sjors
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202364764)
> I don't see how it's required to use a fork of Core, unless you insist on running pre-built binaries only. This may be somewhat naive, but it doesn't seem that difficult to encourage users that want SV2 to use v30 with IPC enabled, and then open up issues as bugs turn up
- @marcofleon
Yes, this is somewhat naive. We've discussed this more than a few times and the reality is that people just don't build from source. We've been talking to miners for a long time and telling them "just take Sjors
...
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286387382)
Agreed, would be nice to have each chain's params be a `constexpr` expression tree in an ideal world.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286387382)
Agreed, would be nice to have each chain's params be a `constexpr` expression tree in an ideal world.
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286383684)
That might be a cool higher level change, although hopefully a commitment will mismatch before we reach the final header.
Hm... thinking through the current code - given 15'000 headers and a period of 600, we have 38 commitments. So there's a 1 in 2^38 chance that all commitments will match. There's a 1/600 chance that `HeadersSyncState::commit_offset` will be 599, in which case `second_chain` would be too short to check the last commitment, making it a 1 in 2^37 chance of test failure for th
...
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286383684)
That might be a cool higher level change, although hopefully a commitment will mismatch before we reach the final header.
Hm... thinking through the current code - given 15'000 headers and a period of 600, we have 38 commitments. So there's a 1 in 2^38 chance that all commitments will match. There's a 1/600 chance that `HeadersSyncState::commit_offset` will be 599, in which case `second_chain` would be too short to check the last commitment, making it a 1 in 2^37 chance of test failure for th
...
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286389214)
Giving us 2x mainnet should give the fuzzers enough time to break things before we reach that level. See 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b for an example of how gradually these values change for 1 release.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286389214)
Giving us 2x mainnet should give the fuzzers enough time to break things before we reach that level. See 11a2d3a63e90cdc1920ede3c67d52a9c72860e6b for an example of how gradually these values change for 1 release.
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286437807)
Agree here, it seems to be a more taxing test to not pretend like there are any more headers, that `HeadersSyncState` will have to make do. Will consider changing this if I push, although it is the same on master. Maybe making it random or doing a `for x in {true, false}` loop over it would be even better.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286437807)
Agree here, it seems to be a more taxing test to not pretend like there are any more headers, that `HeadersSyncState` will have to make do. Will consider changing this if I push, although it is the same on master. Maybe making it random or doing a `for x in {true, false}` loop over it would be even better.
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286427414)
Seems related to your previous GH-comment. Current PR version has the comment:
https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/test/headers_sync_chainwork_tests.cpp#L153
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286427414)
Seems related to your previous GH-comment. Current PR version has the comment:
https://github.com/bitcoin/bitcoin/blob/53341ea10dc2f7df371b416060863bbc094b8773/src/test/headers_sync_chainwork_tests.cpp#L153
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286420829)
I think the idea is to pretend that our peer has longer chain, meaning they are able to send us more headers, it's just that `HeadersSyncState` should be detecting that we've met the threshold and switch to `REDOWNLOAD` even though we tempt it that there are more headers in case it erroneously wants to continue `PRESYNC`.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286420829)
I think the idea is to pretend that our peer has longer chain, meaning they are able to send us more headers, it's just that `HeadersSyncState` should be detecting that we've met the threshold and switch to `REDOWNLOAD` even though we tempt it that there are more headers in case it erroneously wants to continue `PRESYNC`.
💬 murchandamus commented on pull request "wallet: Remove isminetypes":
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3202412412)
ACK be776a1443fdf1a72e0d363c1566d71cb0cda8b5
(https://github.com/bitcoin/bitcoin/pull/32523#issuecomment-3202412412)
ACK be776a1443fdf1a72e0d363c1566d71cb0cda8b5
💬 luke-jr commented on pull request "Avoid file overwriting in fallback `AllocateFileRange` implementation":
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2286476952)
TIL that fseek...SEEK_END is undefined behaviour for binary files.
https://wiki.sei.cmu.edu/confluence/display/c/FIO19-C.+Do+not+use+fseek%28%29+and+ftell%28%29+to+compute+the+size+of+a+regular+file
(https://github.com/bitcoin/bitcoin/pull/33164#discussion_r2286476952)
TIL that fseek...SEEK_END is undefined behaviour for binary files.
https://wiki.sei.cmu.edu/confluence/display/c/FIO19-C.+Do+not+use+fseek%28%29+and+ftell%28%29+to+compute+the+size+of+a+regular+file
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202473607)
@theuni Thanks for stating your preference. If I am following, you would like to:
1. Link cap'n proto and libmultiprocess into `bitcoind` and expose a new `-ipcbind` option there instead of only exposing it through the new `bitcoin` binary.
2. Revert or partially revert #31375 and stop including the `bitcoin` wrapper binary in releases because its presence would be too confusing.
3. Keep all the changes implemented in this PR which toggle the ENABLE_IPC option.
4. Introduce a new ENABLE_
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3202473607)
@theuni Thanks for stating your preference. If I am following, you would like to:
1. Link cap'n proto and libmultiprocess into `bitcoind` and expose a new `-ipcbind` option there instead of only exposing it through the new `bitcoin` binary.
2. Revert or partially revert #31375 and stop including the `bitcoin` wrapper binary in releases because its presence would be too confusing.
3. Keep all the changes implemented in this PR which toggle the ENABLE_IPC option.
4. Introduce a new ENABLE_
...
📝 murchandamus opened a pull request: "coinselection: Tiebreak SRD eviction by weight"
(https://github.com/bitcoin/bitcoin/pull/33223)
@yancyribbens [pointed out](https://github.com/p2pderivatives/rust-bitcoin-coin-selection/pull/108#issuecomment-3202107069) that SRD would fail to find a possible solution if there are multiple UTXOs of the same effective value with diverse weight.
This adds a tiebreaker that will make SRD succeed in such a scenario.
(https://github.com/bitcoin/bitcoin/pull/33223)
@yancyribbens [pointed out](https://github.com/p2pderivatives/rust-bitcoin-coin-selection/pull/108#issuecomment-3202107069) that SRD would fail to find a possible solution if there are multiple UTXOs of the same effective value with diverse weight.
This adds a tiebreaker that will make SRD succeed in such a scenario.
💬 murchandamus commented on pull request "coinselection: Tiebreak SRD eviction by weight":
(https://github.com/bitcoin/bitcoin/pull/33223#discussion_r2286539756)
Move to `coinselection_tests.cpp`
(https://github.com/bitcoin/bitcoin/pull/33223#discussion_r2286539756)
Move to `coinselection_tests.cpp`
💬 luke-jr commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3203396013)
The root issue here appears to be that `ChainstateLoadResult CompleteChainstateInitialization` calls `LoadGenesisBlock` before the chainstate is detected as corrupt, which then clobbers blk00000.dat via `AllocateFileRange` as it (re-)writes the unindexed genesis block to disk.
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3203396013)
The root issue here appears to be that `ChainstateLoadResult CompleteChainstateInitialization` calls `LoadGenesisBlock` before the chainstate is detected as corrupt, which then clobbers blk00000.dat via `AllocateFileRange` as it (re-)writes the unindexed genesis block to disk.
💬 sipa commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3203701690)
Yes, I don't think the `bitcoin` binary as such is an issue. If we end up going for a "`-ipcbind` in `bitcoind`" approach, and we'd have chosen that earlier, that would have removed some of the impetus for adding the `bitcoin` binary - but it seems independently useful to have something that wraps even just `bitcoind` and `bitcoin-cli`.
> your and sipa's approach would bake IPC features ... into all binaries when their only practical use is for the mining interface.
I know you disagree her
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3203701690)
Yes, I don't think the `bitcoin` binary as such is an issue. If we end up going for a "`-ipcbind` in `bitcoind`" approach, and we'd have chosen that earlier, that would have removed some of the impetus for adding the `bitcoin` binary - but it seems independently useful to have something that wraps even just `bitcoind` and `bitcoin-cli`.
> your and sipa's approach would bake IPC features ... into all binaries when their only practical use is for the mining interface.
I know you disagree her
...
💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3203938214)
Thanks again for clarifying sipa. I believe adding the `-ipcbind` to `bitcoind` and having separate build flags for IPC and multiprocess features is a reasonable approach and something I want to propose and discuss further in #30983.
I think we both agree that change could be a followup for a future release and does not need to block the current PR, **but please let me know if I misunderstood and that is not the case, or if you changed your mind about this**.
I think adding `-ipcbind` to `
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3203938214)
Thanks again for clarifying sipa. I believe adding the `-ipcbind` to `bitcoind` and having separate build flags for IPC and multiprocess features is a reasonable approach and something I want to propose and discuss further in #30983.
I think we both agree that change could be a followup for a future release and does not need to block the current PR, **but please let me know if I misunderstood and that is not the case, or if you changed your mind about this**.
I think adding `-ipcbind` to `
...
💬 davidgumberg commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286845684)
My bad, what I mean is that this comment:
```
// Pretend the message is still "full", so we don't abort.
```
Belongs to the `ProcessNextHeaders` above.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2286845684)
My bad, what I mean is that this comment:
```
// Pretend the message is still "full", so we don't abort.
```
Belongs to the `ProcessNextHeaders` above.