⚠️ okabri opened an issue: "Btc"
(https://github.com/bitcoin/bitcoin/issues/30274)
(https://github.com/bitcoin/bitcoin/issues/30274)
👍 ryanofsky approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2113194341)
Code review ACK 4ca48b328ba601299fc5aa4efc9749420590a022. Just rebase and a change to internal commits (removing DoWarning earlier) since last review.
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2113194341)
Code review ACK 4ca48b328ba601299fc5aa4efc9749420590a022. Just rebase and a change to internal commits (removing DoWarning earlier) since last review.
✅ fanquake closed an issue: "Btc"
(https://github.com/bitcoin/bitcoin/issues/30274)
(https://github.com/bitcoin/bitcoin/issues/30274)
:lock: fanquake locked an issue: "Btc"
(https://github.com/bitcoin/bitcoin/issues/30274)
(https://github.com/bitcoin/bitcoin/issues/30274)
👍 ryanofsky approved a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2113216701)
Code review ACK ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883. This is an improvement over the status quo in the normal case, though it seems like it introduces some odd behavior if snapshot block is not on the most work chain, as described in other comments. I think it would be nice to clean the code up more and clarify what is trying to do, instead of merging this change as it is. But it would also be ok to merge in its current form, if you prefer that.
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2113216701)
Code review ACK ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883. This is an improvement over the status quo in the normal case, though it seems like it introduces some odd behavior if snapshot block is not on the most work chain, as described in other comments. I think it would be nice to clean the code up more and clarify what is trying to do, instead of merging this change as it is. But it would also be ok to merge in its current form, if you prefer that.
🤔 BrandonOdiwuor reviewed a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2113239131)
Code Review ACK cbc604f6a7739bf22a5d811b6366453860d59f4c
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2113239131)
Code Review ACK cbc604f6a7739bf22a5d811b6366453860d59f4c
💬 BrandonOdiwuor commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1636599996)
Nit: Would it be better to include the base block hash on the error message to help in debugging
```cpp
throw JSONRPCError(
RPC_INTERNAL_ERROR,
"The base block header (%s) is part of an invalid chain.", base_blockhash.ToString());
```
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1636599996)
Nit: Would it be better to include the base block hash on the error message to help in debugging
```cpp
throw JSONRPCError(
RPC_INTERNAL_ERROR,
"The base block header (%s) is part of an invalid chain.", base_blockhash.ToString());
```
👍 ryanofsky approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2113260997)
Code review ACK 4f9c3369a453f7e5f0c12179dd629ee8fc042379. Only changes since last review are: restoring ApplyArgsManOptions function and inconclusive-not-best-prevblk check order, splitting up an internal commit, and improving some commit messages.
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2113260997)
Code review ACK 4f9c3369a453f7e5f0c12179dd629ee8fc042379. Only changes since last review are: restoring ApplyArgsManOptions function and inconclusive-not-best-prevblk check order, splitting up an internal commit, and improving some commit messages.
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2163209582)
Not sure what is wrong with Apple, but I don't have any insight into their XCode/Apple-Clang versioning and source code, so I think I'll just clarify that this needs Apple-Clang 16 (the meaning of which is unclear).
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2163209582)
Not sure what is wrong with Apple, but I don't have any insight into their XCode/Apple-Clang versioning and source code, so I think I'll just clarify that this needs Apple-Clang 16 (the meaning of which is unclear).
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2163230498)
Ref:
```
/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/map:1282:24: note: in instantiation of function template specialization 'std::__tree<std::__value_type<std::string_view, FuzzTarget>, std::__map_value_compare<std::string_view, std::__value_type<std::string_view, FuzzTarget>, std::less<std::string_view>>, std::allocator<std::__value_type<std::string_view, FuzzTarget>>>::__emplace_unique_key_args<std::string_view, co
...
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2163230498)
Ref:
```
/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/map:1282:24: note: in instantiation of function template specialization 'std::__tree<std::__value_type<std::string_view, FuzzTarget>, std::__map_value_compare<std::string_view, std::__value_type<std::string_view, FuzzTarget>, std::less<std::string_view>>, std::allocator<std::__value_type<std::string_view, FuzzTarget>>>::__emplace_unique_key_args<std::string_view, co
...
💬 ryanofsky commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2163278278)
re: https://github.com/bitcoin/bitcoin/issues/30262#issue-2344196072
> I vaguely remember an earlier discussion about this, but can't find it.
I'm not sure if this is is what you were thinking ofm but we previously discussed "adding helper applications alongside the main application" in a single DMG on macos in https://github.com/bitcoin-core/gui/pull/414. But these were testnet/regtest versions of the GUI, not command line apps.
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2163278278)
re: https://github.com/bitcoin/bitcoin/issues/30262#issue-2344196072
> I vaguely remember an earlier discussion about this, but can't find it.
I'm not sure if this is is what you were thinking ofm but we previously discussed "adding helper applications alongside the main application" in a single DMG on macos in https://github.com/bitcoin-core/gui/pull/414. But these were testnet/regtest versions of the GUI, not command line apps.
👍 TheCharlatan approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2113334850)
Re-ACK 4ca48b328ba601299fc5aa4efc9749420590a022
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2113334850)
Re-ACK 4ca48b328ba601299fc5aa4efc9749420590a022
📝 ismaelsadeeq opened a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275)
Fixes #30009
This PR changes the `estimatesmartfee` default mode to `economical`. Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.
For an in-depth analysis of how significantly the `conservative` mode overestimates, see https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.
(https://github.com/bitcoin/bitcoin/pull/30275)
Fixes #30009
This PR changes the `estimatesmartfee` default mode to `economical`. Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.
For an in-depth analysis of how significantly the `conservative` mode overestimates, see https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.
💬 achow101 commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2163314131)
> Including them in the normal download zip, perhaps in a folder called "Utilities", so that it's visually distinct from the GUI icon.
I think they would still have to be code signed and that could be slightly problematic? AFAIK code signing on MacOS currently requires extra data that exists in the app bundle that would not be available to standalone binaries.
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2163314131)
> Including them in the normal download zip, perhaps in a folder called "Utilities", so that it's visually distinct from the GUI icon.
I think they would still have to be code signed and that could be slightly problematic? AFAIK code signing on MacOS currently requires extra data that exists in the app bundle that would not be available to standalone binaries.
💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1636684790)
> And you seem to be suggesting:
Loading a snapshot should force a node to only consider chains that include the snapshot block and ignore other chains, even if they have more work.
This was meant more as a description of the status quo than a description of the ideal world. I think that with today's logic we don't really have another choice but to do so - temporarily until the background sync has finished.
> but the snapshot block is not on the most-work chain seems different and worth t
...
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1636684790)
> And you seem to be suggesting:
Loading a snapshot should force a node to only consider chains that include the snapshot block and ignore other chains, even if they have more work.
This was meant more as a description of the status quo than a description of the ideal world. I think that with today's logic we don't really have another choice but to do so - temporarily until the background sync has finished.
> but the snapshot block is not on the most-work chain seems different and worth t
...
💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2163327804)
> I think it would be nice to clean the code up more and clarify what is trying to do, instead of merging this change as it is.
I will look into improving this (see thread above) in the next days, so hold off merging please.
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2163327804)
> I think it would be nice to clean the code up more and clarify what is trying to do, instead of merging this change as it is.
I will look into improving this (see thread above) in the next days, so hold off merging please.
💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2163333060)
review ACK 9eea51d9058ad638861aa4b94c1c6e71caeb8765
lgtm
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2163333060)
review ACK 9eea51d9058ad638861aa4b94c1c6e71caeb8765
lgtm
👋 m3dwards's pull request is ready for review: "ci: move ASan job to GitHub Actions from Cirrus CI"
(https://github.com/bitcoin/bitcoin/pull/30193)
(https://github.com/bitcoin/bitcoin/pull/30193)
👍 marcofleon approved a pull request: "Lint: Support running individual lint checks"
(https://github.com/bitcoin/bitcoin/pull/30219#pullrequestreview-2113458095)
Tested ACK 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784. Ran `cargo run` with various of the individual tests and with bad input. Also ran it with no arguments. Everything works as expected and help message looks good.
(https://github.com/bitcoin/bitcoin/pull/30219#pullrequestreview-2113458095)
Tested ACK 0fcbfdb7ad172e518a10dd6e5be4cb6bb1158784. Ran `cargo run` with various of the individual tests and with bad input. Also ran it with no arguments. Everything works as expected and help message looks good.
📝 fanquake opened a pull request: "doc: archive release notes for v27.1"
(https://github.com/bitcoin/bitcoin/pull/30276)
(https://github.com/bitcoin/bitcoin/pull/30276)
💬 fanquake commented on pull request "Update minisketch subtree to eb37a9b8e79f9e49d73b96a49bf97a96d9eb676c":
(https://github.com/bitcoin/bitcoin/pull/30270#issuecomment-2163459777)
Guix Build (aarch64):
```bash
043f66f1a8d58b8dcba45fe88bc1fd0e8cd2222d05a678e3f14bcd52c905533e guix-build-89464ad59cf1/output/aarch64-linux-gnu/SHA256SUMS.part
11f73219fdfe67f878aaf9552d30076267e6925cfac82cf1dfd276b4bc416a38 guix-build-89464ad59cf1/output/aarch64-linux-gnu/bitcoin-89464ad59cf1-aarch64-linux-gnu-debug.tar.gz
ac903add0b8fe9fa3eb578e2270a64c6f7ed1ec054069647a40c564d5b6d0c3c guix-build-89464ad59cf1/output/aarch64-linux-gnu/bitcoin-89464ad59cf1-aarch64-linux-gnu.tar.gz
c9ae08
...
(https://github.com/bitcoin/bitcoin/pull/30270#issuecomment-2163459777)
Guix Build (aarch64):
```bash
043f66f1a8d58b8dcba45fe88bc1fd0e8cd2222d05a678e3f14bcd52c905533e guix-build-89464ad59cf1/output/aarch64-linux-gnu/SHA256SUMS.part
11f73219fdfe67f878aaf9552d30076267e6925cfac82cf1dfd276b4bc416a38 guix-build-89464ad59cf1/output/aarch64-linux-gnu/bitcoin-89464ad59cf1-aarch64-linux-gnu-debug.tar.gz
ac903add0b8fe9fa3eb578e2270a64c6f7ed1ec054069647a40c564d5b6d0c3c guix-build-89464ad59cf1/output/aarch64-linux-gnu/bitcoin-89464ad59cf1-aarch64-linux-gnu.tar.gz
c9ae08
...