💬 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
🤔 tdb3 reviewed a pull request: "Always show 100% verification progress when done"
(https://github.com/bitcoin/bitcoin/pull/30293#pullrequestreview-2121441221)
Thanks for taking a look and presenting the idea.
With bitcoin providing an RPC interface (and the OS providing general process run state), sifting through the debug.log seems like a non-ideal method for status gathering. Are there some examples from out in the wild where this is relied upon?
A cool feature of the RPC interface is that it provides early feedback (e.g. that blocks are being verified). Here are some screenshots of what the RPC caller sees and what is occurring in the debug
...
(https://github.com/bitcoin/bitcoin/pull/30293#pullrequestreview-2121441221)
Thanks for taking a look and presenting the idea.
With bitcoin providing an RPC interface (and the OS providing general process run state), sifting through the debug.log seems like a non-ideal method for status gathering. Are there some examples from out in the wild where this is relied upon?
A cool feature of the RPC interface is that it provides early feedback (e.g. that blocks are being verified). Here are some screenshots of what the RPC caller sees and what is occurring in the debug
...
💬 hMsats commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171658767)
@tdb3
> There might be value in providing a definitive 100% message, but more for general accuracy to bitcoin itself rather than to machine observers of the debug.log.
That was the real reason for this pull request. My example was just a side note. Seeing the 100% is useful also for users looking at the `debug.log`.
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171658767)
@tdb3
> There might be value in providing a definitive 100% message, but more for general accuracy to bitcoin itself rather than to machine observers of the debug.log.
That was the real reason for this pull request. My example was just a side note. Seeing the 100% is useful also for users looking at the `debug.log`.
💬 hMsats commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171677111)
@pinheadmz Ah, maybe we need different strings here. Instead of "Verification progress" maybe "Validation progress" in order not to confuse the two processes.
Linux command: `ag "Verification progress"` gives (including the 100%):
```
bitcoin/test/functional/interface_bitcoin_cli.py
184: assert_equal(cli_get_info['Verification progress'], "%.4f%%" % (blockchain_info['verificationprogress'] * 100))
bitcoin/src/validation.cpp
4613: LogPrintf("Verification progress: 0%%\n");
...
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171677111)
@pinheadmz Ah, maybe we need different strings here. Instead of "Verification progress" maybe "Validation progress" in order not to confuse the two processes.
Linux command: `ag "Verification progress"` gives (including the 100%):
```
bitcoin/test/functional/interface_bitcoin_cli.py
184: assert_equal(cli_get_info['Verification progress'], "%.4f%%" % (blockchain_info['verificationprogress'] * 100))
bitcoin/src/validation.cpp
4613: LogPrintf("Verification progress: 0%%\n");
...
💬 Prabhat1308 commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1641905080)
The number of parents per peer should mostly be in the range of (0-2/3) , This should include a significant numbers of peers to reconcile with . A constant number might not be ideal considering if the array returned is smaller than that constant number , we might be reconciling with non-existent peers .
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1641905080)
The number of parents per peer should mostly be in the range of (0-2/3) , This should include a significant numbers of peers to reconcile with . A constant number might not be ideal considering if the array returned is smaller than that constant number , we might be reconciling with non-existent peers .
💬 pinheadmz commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171746348)
> wait for Bitcoin Core to finish
At this point in your script do you mean
- "wait for Bitcoin Core to be ready for RPC commands" or
- "wait for Bitcoin Core to sync to the most work chain it can find on the network so my UTXO state is most likely to be globally accurate"?
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171746348)
> wait for Bitcoin Core to finish
At this point in your script do you mean
- "wait for Bitcoin Core to be ready for RPC commands" or
- "wait for Bitcoin Core to sync to the most work chain it can find on the network so my UTXO state is most likely to be globally accurate"?
💬 hMsats commented on pull request "Always show 100% verification progress when done":
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171750527)
@pinheadmz Irrespective of my (simple) shell script, in my opinion Bitcoin Core should always print "Verification progress=100%" or even better "Validation progress=100%" in `debug.log`.
(https://github.com/bitcoin/bitcoin/pull/30293#issuecomment-2171750527)
@pinheadmz Irrespective of my (simple) shell script, in my opinion Bitcoin Core should always print "Verification progress=100%" or even better "Validation progress=100%" in `debug.log`.