💬 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.
📝 l0rinc opened a pull request: "doc: unify `datacarriersize` warning with release notes"
(https://github.com/bitcoin/bitcoin/pull/33224)
Follow-up to https://github.com/bitcoin/bitcoin/pull/32406
---
The [release notes](https://github.com/bitcoin/bitcoin/blob/a189d636184b1c28fa4a325b56c1fab8f44527b1/doc/release-notes-32406.md#L1) claim
> [...] marked as deprecated and are expected to be removed in a future release
but the [warning itself](https://github.com/bitcoin/bitcoin/blob/2885bd0e1c4fc863a7f28ff0fd353f5cffb03442/src/init.cpp#L907) claims
> [...] marked as deprecated. They will be removed in a future version.
...
(https://github.com/bitcoin/bitcoin/pull/33224)
Follow-up to https://github.com/bitcoin/bitcoin/pull/32406
---
The [release notes](https://github.com/bitcoin/bitcoin/blob/a189d636184b1c28fa4a325b56c1fab8f44527b1/doc/release-notes-32406.md#L1) claim
> [...] marked as deprecated and are expected to be removed in a future release
but the [warning itself](https://github.com/bitcoin/bitcoin/blob/2885bd0e1c4fc863a7f28ff0fd353f5cffb03442/src/init.cpp#L907) claims
> [...] marked as deprecated. They will be removed in a future version.
...
💬 hebasto commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3204097881)
Some random guy suggests setting an environment variable: https://youtu.be/xNHKTdnn4fY?t=1535 :)
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3204097881)
Some random guy suggests setting an environment variable: https://youtu.be/xNHKTdnn4fY?t=1535 :)
💬 hodlinator commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3204431479)
> Some random guy suggests setting an environment variable: https://youtu.be/xNHKTdnn4fY?t=1535 :)
We shouldn't expect the average Bitcoin Core contributor to consume 90min CMake videos but it's great that you do.
He also suggests every tool should support compile_commands.json and any issue/suggestion to do so would be massively upvoted.
(Adjacent issue: I never build within my IDE, so even if it were to set the environment variable automatically, I would not have benefited from that).
...
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3204431479)
> Some random guy suggests setting an environment variable: https://youtu.be/xNHKTdnn4fY?t=1535 :)
We shouldn't expect the average Bitcoin Core contributor to consume 90min CMake videos but it's great that you do.
He also suggests every tool should support compile_commands.json and any issue/suggestion to do so would be massively upvoted.
(Adjacent issue: I never build within my IDE, so even if it were to set the environment variable automatically, I would not have benefited from that).
...
💬 hodlinator commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2287189020)
Base version for this PR: https://github.com/bitcoin/bitcoin/blob/fa05a726c225dc65dee79367bb67f099ae4f99e6/src/test/headers_sync_chainwork_tests.cpp#L95-L98
That comment would indeed be more useful before the first `ProcessNextHeaders`-call. Will correct if I push again.
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2287189020)
Base version for this PR: https://github.com/bitcoin/bitcoin/blob/fa05a726c225dc65dee79367bb67f099ae4f99e6/src/test/headers_sync_chainwork_tests.cpp#L95-L98
That comment would indeed be more useful before the first `ProcessNextHeaders`-call. Will correct if I push again.
💬 davidgumberg commented on pull request "build: Enable -DCMAKE_EXPORT_COMPILE_COMMANDS=1":
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3204539562)
I think that knobs should be set in a way that as much as is reasonable relieves users from the burden of having to pore over [documentation](https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html) to understand what each option does before proceeding successfully, and I maintain that no one has or ever would complain if this was enabled[^1], in this project or in any other project, and many would enjoy living life in sweet ignorance of `CMAKE_EXPORT_COMPILE_COMMANDS=1` while reaping
...
(https://github.com/bitcoin/bitcoin/pull/33187#issuecomment-3204539562)
I think that knobs should be set in a way that as much as is reasonable relieves users from the burden of having to pore over [documentation](https://cmake.org/cmake/help/latest/manual/cmake-variables.7.html) to understand what each option does before proceeding successfully, and I maintain that no one has or ever would complain if this was enabled[^1], in this project or in any other project, and many would enjoy living life in sweet ignorance of `CMAKE_EXPORT_COMPILE_COMMANDS=1` while reaping
...
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3204696862)
Thank you for the review @l0rinc!
Rebased 254d0a75b50b0eaf91003ea8a0534981ec740090 -> fc07ce3718b5b8cc168ab634885e0317b9621e8c ([blocktreestore_2](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_2) -> [blocktreestore_3](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_2..blocktreestore_3))
Updated fc07ce3718b5b8cc168ab634885e0317b9621e8c -> d35ceaeb463bc836ac4fc4bd6dd4f387647f33fb ([blocktre
...
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-3204696862)
Thank you for the review @l0rinc!
Rebased 254d0a75b50b0eaf91003ea8a0534981ec740090 -> fc07ce3718b5b8cc168ab634885e0317b9621e8c ([blocktreestore_2](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_2) -> [blocktreestore_3](https://github.com/TheCharlatan/bitcoin/tree/blocktreestore_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/blocktreestore_2..blocktreestore_3))
Updated fc07ce3718b5b8cc168ab634885e0317b9621e8c -> d35ceaeb463bc836ac4fc4bd6dd4f387647f33fb ([blocktre
...
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2285423317)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2285423317)
Fixed.
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2285417562)
Yes, removed.
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2285417562)
Yes, removed.
💬 TheCharlatan commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2285428314)
It would be nice if `GetSerializeSize` would be a constexpr. But yes, moving this out.
(https://github.com/bitcoin/bitcoin/pull/32427#discussion_r2285428314)
It would be nice if `GetSerializeSize` would be a constexpr. But yes, moving this out.