💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2304200104)
`4533445fd7...1dcd626fe1`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2304200104)
`4533445fd7...1dcd626fe1`: rebase due to conflicts
💬 sipa commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2304213364)
@maflcko Ah yes, it is the version here that had a corruption in the txindex: https://bitcoin.stackexchange.com/q/123999/208
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2304213364)
@maflcko Ah yes, it is the version here that had a corruption in the txindex: https://bitcoin.stackexchange.com/q/123999/208
👍 TheCharlatan approved a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2254019496)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
I reviewed most of this and at least glanced over every pull request in hebasto's repository since the project started review in his repository early last year. The change does contain some tradeoffs compared to the current build system, most of which are inherent to cmake and challenges that are also faced by other projects. However none of these are serious compared to the benefits that this migration will bring. I hope the better IDE integration
...
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2254019496)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4
I reviewed most of this and at least glanced over every pull request in hebasto's repository since the project started review in his repository early last year. The change does contain some tradeoffs compared to the current build system, most of which are inherent to cmake and challenges that are also faced by other projects. However none of these are serious compared to the benefits that this migration will bring. I hope the better IDE integration
...
💬 vasild commented on issue "-proxy does not work for addresses like 10.x.y.z":
(https://github.com/bitcoin/bitcoin/issues/25684#issuecomment-2304235136)
I agree that this is unexpected and should be documented or fixed or made optional.
> One more side effect of this: bitcoind may leak information (like - is bitcoin node running at 192.168.0.1) to outside world.
How?
> The question to ask (in my opinion) is why bitcoind should know about unroutable addresses at all?
Why those addresses are treated in a special way? It's not bitcoind's job to know routing rules.
Bitcoin nodes keep a list of possible addresses of other nodes to connec
...
(https://github.com/bitcoin/bitcoin/issues/25684#issuecomment-2304235136)
I agree that this is unexpected and should be documented or fixed or made optional.
> One more side effect of this: bitcoind may leak information (like - is bitcoin node running at 192.168.0.1) to outside world.
How?
> The question to ask (in my opinion) is why bitcoind should know about unroutable addresses at all?
Why those addresses are treated in a special way? It's not bitcoind's job to know routing rules.
Bitcoin nodes keep a list of possible addresses of other nodes to connec
...
⚠️ vasild opened an issue: "Split socket handling out of CConnman"
(https://github.com/bitcoin/bitcoin/issues/30694)
### Please describe the feature you'd like to see added.
Split `CConnman` in two pieces:
* protocol agnostic low level socket and connection handler and
* bitcoin-p2p protocol specific handler.
This way the first part can be reused for Stratum V2 and for libevent-less RPC/HTTP server.
### Is your feature related to a problem, if so please describe it.
Doing Stratum V2 and removing libevent from RPC requires duplicating some of the code from `CConnman` - binding and listening and the se
...
(https://github.com/bitcoin/bitcoin/issues/30694)
### Please describe the feature you'd like to see added.
Split `CConnman` in two pieces:
* protocol agnostic low level socket and connection handler and
* bitcoin-p2p protocol specific handler.
This way the first part can be reused for Stratum V2 and for libevent-less RPC/HTTP server.
### Is your feature related to a problem, if so please describe it.
Doing Stratum V2 and removing libevent from RPC requires duplicating some of the code from `CConnman` - binding and listening and the se
...
💬 vasild commented on issue "Split socket handling out of CConnman":
(https://github.com/bitcoin/bitcoin/issues/30694#issuecomment-2304255853)
I am looking into doing this.
cc @Sjors, @pinheadmz
(https://github.com/bitcoin/bitcoin/issues/30694#issuecomment-2304255853)
I am looking into doing this.
cc @Sjors, @pinheadmz
💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726763236)
> Your concern here is that this may not work (or not work as expected) on the full range of environments where we expect depends-builds to work?
Yea. It's also somewhat displayed by the inconsistency of the change here; `C/XXFLAGS` is being put into depends, but the `LDFLAGS` are put into Guix. The LDFLAGS can't be hard-coded into depends, because that'll break depends builds where the libc/other base libs haven't been compiled with support for CET (for example, the current release of Alpine
...
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726763236)
> Your concern here is that this may not work (or not work as expected) on the full range of environments where we expect depends-builds to work?
Yea. It's also somewhat displayed by the inconsistency of the change here; `C/XXFLAGS` is being put into depends, but the `LDFLAGS` are put into Guix. The LDFLAGS can't be hard-coded into depends, because that'll break depends builds where the libc/other base libs haven't been compiled with support for CET (for example, the current release of Alpine
...
💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726766583)
I don't think splitting flags into `LDFLAGS` and `HARDENED_LDFLAGS` inside Guix makes sense, as, everything in Guix is meant to be a "hardened" build.
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1726766583)
I don't think splitting flags into `LDFLAGS` and `HARDENED_LDFLAGS` inside Guix makes sense, as, everything in Guix is meant to be a "hardened" build.
💬 pinheadmz commented on issue "Split socket handling out of CConnman":
(https://github.com/bitcoin/bitcoin/issues/30694#issuecomment-2304298441)
> I am looking into doing this.
Wow thanks!
My HTTP branch is almost done but is very messy. You can see me copy/paste/edit socket handling in
https://github.com/pinheadmz/bitcoin/blob/http-netbase/src/http.cpp
`GenerateEventsPerSock()`
`HandleConnections()`
`AcceptConnections()`
etc...
(https://github.com/bitcoin/bitcoin/issues/30694#issuecomment-2304298441)
> I am looking into doing this.
Wow thanks!
My HTTP branch is almost done but is very messy. You can see me copy/paste/edit socket handling in
https://github.com/pinheadmz/bitcoin/blob/http-netbase/src/http.cpp
`GenerateEventsPerSock()`
`HandleConnections()`
`AcceptConnections()`
etc...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726799882)
I'm open to the idea, but I don't immediately see a super elegant solution. Thoughts:
- a lot of the `::size() * 2` uses are templated (such as in the referenced example here), so constants wouldn't really work. Adding a `base_blob::hex_size()` seems inappropriate to me too, that's not really a `base_blob` concern.
- non-templated use is not very frequent, and mostly in tests. Potentially a `constexpr auto UINT256_HEX_SIZE{uint256::size() * 2}` could work, but I'm not really convinced that's w
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726799882)
I'm open to the idea, but I don't immediately see a super elegant solution. Thoughts:
- a lot of the `::size() * 2` uses are templated (such as in the referenced example here), so constants wouldn't really work. Adding a `base_blob::hex_size()` seems inappropriate to me too, that's not really a `base_blob` concern.
- non-templated use is not very frequent, and mostly in tests. Potentially a `constexpr auto UINT256_HEX_SIZE{uint256::size() * 2}` could work, but I'm not really convinced that's w
...
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726804057)
This behaviour was mentioned [here](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714546457) (and correctly identified as broken by a previous version of this PR), so that's why I added a test to make it explicit. I don't have a strong view on whether that behaviour should be allowed, but since it exists, and some people seem to find it useful, and we're trying to minimize breaking behaviour now, I think I'd prefer keeping it and documenting it.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726804057)
This behaviour was mentioned [here](https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714546457) (and correctly identified as broken by a previous version of this PR), so that's why I added a test to make it explicit. I don't have a strong view on whether that behaviour should be allowed, but since it exists, and some people seem to find it useful, and we're trying to minimize breaking behaviour now, I think I'd prefer keeping it and documenting it.
💬 glozow commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1726824485)
I'm convinced, marking resolved.
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1726824485)
I'm convinced, marking resolved.
👍 Sjors approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2253922313)
tACK b5292334e5e8792644123b627896359eb1da8d25
I was initially confused because I got this:
```
$ src/bitcoin-cli dumptxoutset $HOME/dev/utxo-snapshots/utxo-840000-pr29553.dat rollback
error code: -1
error message:
Internal bug detected: snapshot_heights.size() > 0
rpc/blockchain.cpp:2738 (operator())
Bitcoin Core v27.99.0-b5292334e5e8
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
````
But that's because this PR wasn't rebased after #28553 got merged.
...
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2253922313)
tACK b5292334e5e8792644123b627896359eb1da8d25
I was initially confused because I got this:
```
$ src/bitcoin-cli dumptxoutset $HOME/dev/utxo-snapshots/utxo-840000-pr29553.dat rollback
error code: -1
error message:
Internal bug detected: snapshot_heights.size() > 0
rpc/blockchain.cpp:2738 (operator())
Bitcoin Core v27.99.0-b5292334e5e8
Please report this issue here: https://github.com/bitcoin/bitcoin/issues
````
But that's because this PR wasn't rebased after #28553 got merged.
...
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726654514)
bb3981c01985989b42da0a24c027be757ab54612: -> "are marked invalid"
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726654514)
bb3981c01985989b42da0a24c027be757ab54612: -> "are marked invalid"
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726662966)
bb3981c01985989b42da0a24c027be757ab54612 `\"latest\"`?
The first time I read "latest type" as meaning a new type.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726662966)
bb3981c01985989b42da0a24c027be757ab54612 `\"latest\"`?
The first time I read "latest type" as meaning a new type.
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726770642)
b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: a followup could handle multiple blocks at the same height by running a loop that invalidates all (new) tips at the target height that are not the block hash we expected.
Those could be tracked in `invalidate_indexes` and be reconsidered in reverse order (and with the network still off) after the snapshot is made.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726770642)
b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: a followup could handle multiple blocks at the same height by running a loop that invalidates all (new) tips at the target height that are not the block hash we expected.
Those could be tracked in `invalidate_indexes` and be reconsidered in reverse order (and with the network still off) after the snapshot is made.
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726791942)
It seems conceptually simpler to first write the snapshot, reconsider all blocks that were marked invalid and only then restore network activity.
> make the node more usable while the snapshot is being created
Lower usability is probably feature. The user should not be doing anything important until the chain is back at the original tip, e.g. loading a wallet could still cause confusion.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726791942)
It seems conceptually simpler to first write the snapshot, reconsider all blocks that were marked invalid and only then restore network activity.
> make the node more usable while the snapshot is being created
Lower usability is probably feature. The user should not be doing anything important until the chain is back at the original tip, e.g. loading a wallet could still cause confusion.
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726781190)
b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: assuming this is blocking and it could take hours before the throw below, it would be good to emit a log message in the mean time.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726781190)
b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: assuming this is blocking and it could take hours before the throw below, it would be good to emit a log message in the mean time.
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726741569)
b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: I had to read this description several times to get it, but the examples make it clear.
Having a third positional optional argument `height` would have seemed easier to me, but it's fine like this.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726741569)
b23abfca68f7b349e6d2f9e04369f0fdc82d2a98: I had to read this description several times to get it, but the examples make it clear.
Having a third positional optional argument `height` would have seemed easier to me, but it's fine like this.
💬 Sjors commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726658380)
bb3981c01985989b42da0a24c027be757ab54612: may be add: "This inconsistent state is also why network activity is temporarily disabled, causing us to disconnect from all peers."
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1726658380)
bb3981c01985989b42da0a24c027be757ab54612: may be add: "This inconsistent state is also why network activity is temporarily disabled, causing us to disconnect from all peers."