💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1726485189)
> Could make `rand_seed` `const` to clarify usage (approved by clang-format-diff.py):
Makes sense. However, I am not modifying this line of code, so I'll leave the style-nit as-is for now, to keep the already large pull request focussed on only the minimum diff required to fix the found bugs and apply the refactoring to achieve the stated goals in the pull motivation.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1726485189)
> Could make `rand_seed` `const` to clarify usage (approved by clang-format-diff.py):
Makes sense. However, I am not modifying this line of code, so I'll leave the style-nit as-is for now, to keep the already large pull request focussed on only the minimum diff required to fix the found bugs and apply the refactoring to achieve the stated goals in the pull motivation.
💬 maflcko commented on pull request "Fix maybe-uninitialized warning in IsSpentKey":
(https://github.com/bitcoin/bitcoin/pull/30691#issuecomment-2303970906)
lgtm ACK 17707db939cb09a16c002d226152e71fce9289f2
(I haven't tested this, as I don't use GCC)
Seems fine to refactor the code to work around the GCC bug, so that confusion is reduced when using GCC.
An alternative would be to disable the warning completely in GCC, because it is known to be buggy in all versions of GCC and I think has so far not found any real issue, did it?
(https://github.com/bitcoin/bitcoin/pull/30691#issuecomment-2303970906)
lgtm ACK 17707db939cb09a16c002d226152e71fce9289f2
(I haven't tested this, as I don't use GCC)
Seems fine to refactor the code to work around the GCC bug, so that confusion is reduced when using GCC.
An alternative would be to disable the warning completely in GCC, because it is known to be buggy in all versions of GCC and I think has so far not found any real issue, did it?
💬 Sjors 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_r1726615759)
I'll move it to the other commit, or find a way to use it here, if I need to retouch.
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1726615759)
I'll move it to the other commit, or find a way to use it here, if I need to retouch.
💬 Sjors 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_r1726617405)
If we want to simulate a full timewarp attack, we'd need multiple such periods too.
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1726617405)
If we want to simulate a full timewarp attack, we'd need multiple such periods too.
💬 maflcko commented on issue "bitcoind shouldn't be shutdown automatically despite wallet synchronisation error":
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2304153431)
Updating the help texts in the GUI and the daemon around `-prune` to mention that wallets (and indexes) should ideally be kept loaded or active to avoid sync catch-up errors from happening at all and avoid this error in the first place could make sense as well.
(https://github.com/bitcoin/bitcoin/issues/30671#issuecomment-2304153431)
Updating the help texts in the GUI and the daemon around `-prune` to mention that wallets (and indexes) should ideally be kept loaded or active to avoid sync catch-up errors from happening at all and avoid this error in the first place could make sense as well.
💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2304162956)
A reboot fixed the network issue again and I still can not reproduce or otherwise see any corruption on any machine.
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2304162956)
A reboot fixed the network issue again and I still can not reproduce or otherwise see any corruption on any machine.
💬 hebasto commented on pull request "CMake replaces Autotools":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2304196456)
> > It aims reveal conflicts with other PRs.
> > For more details, please refer to today's IRC meeting discussion.
>
> It would be good to mention any important key points inline, otherwise it will be harder to understand whether this is the pull request that will be merged as a follow-up, and whether it should be reviewed, or if there is another place where the review should happen.
The PR description has been updated with mentioning that this PR is not intended to be reviewed/merged in
...
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2304196456)
> > It aims reveal conflicts with other PRs.
> > For more details, please refer to today's IRC meeting discussion.
>
> It would be good to mention any important key points inline, otherwise it will be harder to understand whether this is the pull request that will be merged as a follow-up, and whether it should be reviewed, or if there is another place where the review should happen.
The PR description has been updated with mentioning that this PR is not intended to be reviewed/merged in
...
💬 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.
...