💬 sdaftuar commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2169559245)
Good discussion question, thanks for raising!
> Considering that AssumeUtxo sync is meant to be an optional and temporary optimization, and that large reorgs should be very infrequent, it could also make sense to abandon the AssumeUtxo sync, delete the snapshot chainstate and revert to normal sync as soon as we accept a header on a different chain that has more work than the best header of the snapshot chain.
I think something like this makes more sense than to change how the background sy
...
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2169559245)
Good discussion question, thanks for raising!
> Considering that AssumeUtxo sync is meant to be an optional and temporary optimization, and that large reorgs should be very infrequent, it could also make sense to abandon the AssumeUtxo sync, delete the snapshot chainstate and revert to normal sync as soon as we accept a header on a different chain that has more work than the best header of the snapshot chain.
I think something like this makes more sense than to change how the background sy
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2169622121)
Force push: rebased for small conflict in `src/Makefile.am`, collected fixup commits, no other changes.
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2169622121)
Force push: rebased for small conflict in `src/Makefile.am`, collected fixup commits, no other changes.
💬 brunoerg commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2169980849)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2169980849)
Concept ACK
💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1641289196)
Lol, I even spent a few hours looking directly at this and trying to figure out if this could be a threading problem.
🤦
I suppose I trusted the annotations so much I didn't even notice that the code didn't take the lock. That's embarrassing!
Looks like [this is our issue](https://github.com/llvm/llvm-project/pull/67520).
I'll give that patched branch a shot next week and see if it turns up anything else in our codebase. Will chime in upstream with a +1 as well (assuming it works).
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1641289196)
Lol, I even spent a few hours looking directly at this and trying to figure out if this could be a threading problem.
🤦
I suppose I trusted the annotations so much I didn't even notice that the code didn't take the lock. That's embarrassing!
Looks like [this is our issue](https://github.com/llvm/llvm-project/pull/67520).
I'll give that patched branch a shot next week and see if it turns up anything else in our codebase. Will chime in upstream with a +1 as well (assuming it works).
👋 tdb3's pull request is ready for review: "test: write functional test results to csv"
(https://github.com/bitcoin/bitcoin/pull/30291)
(https://github.com/bitcoin/bitcoin/pull/30291)
👍 tdb3 approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2120845152)
ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df
Great job laying groundwork for Sv2.
Left some comments (mostly nits). Ran unit and functional tests (passed).
Other than running unit/functional tests and exercising test networks (e.g. regtest for `generate` calls), any recommendations on other good ways to exercise these changes? `getblocktemplate()` is a pretty crucial RPC, so being extra paranoid here could be prudent.
May want to update the description, since RPC functions beyond `ge
...
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2120845152)
ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df
Great job laying groundwork for Sv2.
Left some comments (mostly nits). Ran unit and functional tests (passed).
Other than running unit/functional tests and exercising test networks (e.g. regtest for `generate` calls), any recommendations on other good ways to exercise these changes? `getblocktemplate()` is a pretty crucial RPC, so being extra paranoid here could be prudent.
May want to update the description, since RPC functions beyond `ge
...
💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641440653)
Although extraordinarily improbable, is the value of 0 reserved as an invalid hash (maybe I'm forgetting something)? If not, maybe it's worth updating this function to return something like `std::optional<uint256>` instead?
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641440653)
Although extraordinarily improbable, is the value of 0 reserved as an invalid hash (maybe I'm forgetting something)? If not, maybe it's worth updating this function to return something like `std::optional<uint256>` instead?
💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641438112)
nit: If a default value for `check_merkle_root` is added to `testBlockValidity()`, this call could be updated as well.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641438112)
nit: If a default value for `check_merkle_root` is added to `testBlockValidity()`, this call could be updated as well.
💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641492827)
nit: this comment (and the one on 811) still mention `CreateNewBlock` rather than the new `createNewBlock`. Feel free to ignore unless touching this file for something else.
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641492827)
nit: this comment (and the one on 811) still mention `CreateNewBlock` rather than the new `createNewBlock`. Feel free to ignore unless touching this file for something else.
💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641433677)
nit: Since this is an interface, to maximize clarity it might be good to add Doxygen-ey style `@param` and `@returns` statements in the function comment, similar to `processNewBlock()`.
Same for `createNewBlock()`.
(https://www.doxygen.nl/manual/commands.html#cmdparam)
Also, would it make sense to provide a default value for `check_merkle_root` (e.g. default true)? Or do we want users of the interface to explicitly state whether or not they want merkle root checking performed?
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641433677)
nit: Since this is an interface, to maximize clarity it might be good to add Doxygen-ey style `@param` and `@returns` statements in the function comment, similar to `processNewBlock()`.
Same for `createNewBlock()`.
(https://www.doxygen.nl/manual/commands.html#cmdparam)
Also, would it make sense to provide a default value for `check_merkle_root` (e.g. default true)? Or do we want users of the interface to explicitly state whether or not they want merkle root checking performed?
👍 hebasto approved a pull request: "Enable clang-tidy checks for self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30234#pullrequestreview-2121224489)
ACK 26a7f70b5d2b1cbfbf03e0b57b9b5ea92dafbb19, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30234#pullrequestreview-2121224489)
ACK 26a7f70b5d2b1cbfbf03e0b57b9b5ea92dafbb19, I have reviewed the code and it looks OK.
💬 hebasto commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2171268746)
We faced a similar API-breaking change in libevent. See https://github.com/bitcoin/bitcoin/issues/23606.
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2171268746)
We faced a similar API-breaking change in libevent. See https://github.com/bitcoin/bitcoin/issues/23606.
👍 hebasto approved a pull request: "Revert "contrib: macdeploy: monkey-patch gen-sdk to be deterministic""
(https://github.com/bitcoin/bitcoin/pull/30282#pullrequestreview-2121262101)
ACK b03a45b13e4e33b044cae6c97a6d608f6f3d43f3, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30282#pullrequestreview-2121262101)
ACK b03a45b13e4e33b044cae6c97a6d608f6f3d43f3, I have reviewed the code and it looks OK.
💬 hebasto commented on pull request "Revert "contrib: macdeploy: monkey-patch gen-sdk to be deterministic"":
(https://github.com/bitcoin/bitcoin/pull/30282#issuecomment-2171272886)
cc @prusnak
(https://github.com/bitcoin/bitcoin/pull/30282#issuecomment-2171272886)
cc @prusnak
👍 hebasto approved a pull request: "macOS: rewrite some docs & swap `mmacosx-version-min` for `mmacos-version-min`"
(https://github.com/bitcoin/bitcoin/pull/30287#pullrequestreview-2121294059)
ACK 7c298fe0df38696f60e981469422315c03a722da.
My Guix builds:
```
x86_64
c749e06e582abccb481fed3b1754f209d81a7092840bfe18ee68a68b1cafef0d guix-build-7c298fe0df38/output/arm64-apple-darwin/SHA256SUMS.part
5b0d3d772146694214c6041c299efb05c2f76c9fe7dc80fc522270786f426388 guix-build-7c298fe0df38/output/arm64-apple-darwin/bitcoin-7c298fe0df38-arm64-apple-darwin-unsigned.tar.gz
537b5e3068e4939c36b758dd8957f25ffa66b572663e9bc699ce109857aa1e6a guix-build-7c298fe0df38/output/arm64-apple-darwin
...
(https://github.com/bitcoin/bitcoin/pull/30287#pullrequestreview-2121294059)
ACK 7c298fe0df38696f60e981469422315c03a722da.
My Guix builds:
```
x86_64
c749e06e582abccb481fed3b1754f209d81a7092840bfe18ee68a68b1cafef0d guix-build-7c298fe0df38/output/arm64-apple-darwin/SHA256SUMS.part
5b0d3d772146694214c6041c299efb05c2f76c9fe7dc80fc522270786f426388 guix-build-7c298fe0df38/output/arm64-apple-darwin/bitcoin-7c298fe0df38-arm64-apple-darwin-unsigned.tar.gz
537b5e3068e4939c36b758dd8957f25ffa66b572663e9bc699ce109857aa1e6a guix-build-7c298fe0df38/output/arm64-apple-darwin
...
📝 hMsats opened a pull request: "Always show 100% verification progress when done"
(https://github.com/bitcoin/bitcoin/pull/30293)
External software/scripts may monitor Bitcoin Core's verification progress in debug.log.
Now these have to scan for "No coin database inconsistencies" to see if the verification progress has finished. It's cleaner to always show 100% when the verification progress has finished (without encountering any problems). If I remember correctly this was the case in older versions of Bitcoin Core.
NOW:
```
2024-06-16T09:12:03Z Verification progress: 0%
2024-06-16T09:12:55Z Verification progress:
...
(https://github.com/bitcoin/bitcoin/pull/30293)
External software/scripts may monitor Bitcoin Core's verification progress in debug.log.
Now these have to scan for "No coin database inconsistencies" to see if the verification progress has finished. It's cleaner to always show 100% when the verification progress has finished (without encountering any problems). If I remember correctly this was the case in older versions of Bitcoin Core.
NOW:
```
2024-06-16T09:12:03Z Verification progress: 0%
2024-06-16T09:12:55Z Verification progress:
...
💬 pinheadmz commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171512300)
> External software/scripts may monitor Bitcoin Core's verification progress in debug.log.
Do you run a script like this? I'm curious what the motivation is, since I'm pretty sure if verification fails the program quits.
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171512300)
> External software/scripts may monitor Bitcoin Core's verification progress in debug.log.
Do you run a script like this? I'm curious what the motivation is, since I'm pretty sure if verification fails the program quits.
💬 ryanofsky commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2171582899)
If I can summarize and clarify, neither of you think the current behavior of locking in snapshot block, and temporarily refusing to consider chains that don't include it is a good idea? The list of reasons above trying to justify current behavior are basically B.S.? (The "Possible advantages of original chainstate targeting the snapshot block" section about network hard forks and eclipse attacks)
Instead, the behavior you both seem to prefer is just: when a snapshot is loaded, as soon as a ne
...
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2171582899)
If I can summarize and clarify, neither of you think the current behavior of locking in snapshot block, and temporarily refusing to consider chains that don't include it is a good idea? The list of reasons above trying to justify current behavior are basically B.S.? (The "Possible advantages of original chainstate targeting the snapshot block" section about network hard forks and eclipse attacks)
Instead, the behavior you both seem to prefer is just: when a snapshot is loaded, as soon as a ne
...
💬 hMsats commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171583802)
@pinheadmz yes, I make a system backup every month like this: pre-backup with rsync, turn off my bitcoin node, quick final rsync, restart bitcoin node and wait for Bitcoin Core to finish (while showing verification progress) before starting my public electrum server, lightning node and pruning node (only connected to my main node).
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171583802)
@pinheadmz yes, I make a system backup every month like this: pre-backup with rsync, turn off my bitcoin node, quick final rsync, restart bitcoin node and wait for Bitcoin Core to finish (while showing verification progress) before starting my public electrum server, lightning node and pruning node (only connected to my main node).
💬 pinheadmz commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171615874)
Ok and do you understand that the verification progress you are touching here is in the startup sequence where Bitcoin ensures the database is consistent before even connecting to the network?
It's possible you may be referring to sync progress which is the process of downloading and verifying new blocks until the most work chain has been processed. That progress will never reach 100% by design. There is some relevant discussion here:
https://github.com/bitcoin/bitcoin/issues/28847
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171615874)
Ok and do you understand that the verification progress you are touching here is in the startup sequence where Bitcoin ensures the database is consistent before even connecting to the network?
It's possible you may be referring to sync progress which is the process of downloading and verifying new blocks until the most work chain has been processed. That progress will never reach 100% by design. There is some relevant discussion here:
https://github.com/bitcoin/bitcoin/issues/28847