💬 jonasschnelli commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2273103049)
Right-clicking to bypass Gatekeeper will likely be unavailable in the next version of macOS (Sequoia / 15).
https://developer.apple.com/news/?id=saqachfa
Users can still go to System Settings > Privacy & Security to "review security information" and launch the application regardless of missing notarization.
This might be a good time to reconsider notarization for the project.
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2273103049)
Right-clicking to bypass Gatekeeper will likely be unavailable in the next version of macOS (Sequoia / 15).
https://developer.apple.com/news/?id=saqachfa
Users can still go to System Settings > Privacy & Security to "review security information" and launch the application regardless of missing notarization.
This might be a good time to reconsider notarization for the project.
💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273120946)
> Did you try setting the source dir read-only, and it passes with this change?
Yes, it does. However, such a test is not very useful because Python just silently skips writing the bytecode cache if there are no write permissions.
A more useful test can be conducted on a [branch](https://github.com/hebasto/bitcoin/tree/cmake-kill-autotools), which performs a full migration to CMake and cleans up all `.gitignore` files from Autotools remnants.
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273120946)
> Did you try setting the source dir read-only, and it passes with this change?
Yes, it does. However, such a test is not very useful because Python just silently skips writing the bytecode cache if there are no write permissions.
A more useful test can be conducted on a [branch](https://github.com/hebasto/bitcoin/tree/cmake-kill-autotools), which performs a full migration to CMake and cleans up all `.gitignore` files from Autotools remnants.
🤔 stickies-v reviewed a pull request: "test: Do not write Python bytecode to source directory"
(https://github.com/bitcoin/bitcoin/pull/30533#pullrequestreview-2223973500)
Concept ACK, the performance benefits of caching bytecode are irrelevant here so keeping a clean source dir seems preferable.
As an alternative approach, might it make more sense to use [`pycache_prefix`](https://docs.python.org/3/library/sys.html#sys.pycache_prefix) so we can address this concern globally instead of just for this file, and in the build system code instead of leaking this into non-build system code? And that way we can keep the (in this case negligible) performance benefits
...
(https://github.com/bitcoin/bitcoin/pull/30533#pullrequestreview-2223973500)
Concept ACK, the performance benefits of caching bytecode are irrelevant here so keeping a clean source dir seems preferable.
As an alternative approach, might it make more sense to use [`pycache_prefix`](https://docs.python.org/3/library/sys.html#sys.pycache_prefix) so we can address this concern globally instead of just for this file, and in the build system code instead of leaking this into non-build system code? And that way we can keep the (in this case negligible) performance benefits
...
💬 stickies-v commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#discussion_r1706757028)
Should also update https://github.com/bitcoin/bitcoin/blob/2987ba66375863cdfb0e6065a36c5d3302499c0b/Makefile.am#L332
(https://github.com/bitcoin/bitcoin/pull/30533#discussion_r1706757028)
Should also update https://github.com/bitcoin/bitcoin/blob/2987ba66375863cdfb0e6065a36c5d3302499c0b/Makefile.am#L332
💬 theStack commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273127594)
Not sure if this is a convincing argument for anyone, but I can at least see a point of keeping the block height for purely informative purposes, in case the snapshot is processed by external tools which don't have access to the block index, e.g. https://github.com/bitcoin/bitcoin/pull/27432 (or, at some point even the https://linux.die.net/man/1/file utility, which can not only determine a file type, but also emit a short meta-data summary, if properly implemented in the magic file). Wouldn't i
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273127594)
Not sure if this is a convincing argument for anyone, but I can at least see a point of keeping the block height for purely informative purposes, in case the snapshot is processed by external tools which don't have access to the block index, e.g. https://github.com/bitcoin/bitcoin/pull/27432 (or, at some point even the https://linux.die.net/man/1/file utility, which can not only determine a file type, but also emit a short meta-data summary, if properly implemented in the magic file). Wouldn't i
...
💬 maflcko commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273147602)
> The arguments here don't convince me yet that the height really has to be dropped necessarily but I also seem to be less passionate about this issue than @maflcko et al.
Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed. I haven't tried it, but https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1700350964 (the error message for the user will not be: "Corrupt metadata, block heigh
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273147602)
> The arguments here don't convince me yet that the height really has to be dropped necessarily but I also seem to be less passionate about this issue than @maflcko et al.
Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed. I haven't tried it, but https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1700350964 (the error message for the user will not be: "Corrupt metadata, block heigh
...
💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273161309)
> As an alternative approach, might it make more sense to use [`pycache_prefix`](https://docs.python.org/3/library/sys.html#sys.pycache_prefix) so we can address this concern globally instead of just for this file, and in the build system code instead of leaking this into non-build system code? And that way we can keep the (in this case negligible) performance benefits of caching bytecode?
I did consider it. I have to admit that I failed to find a solution that touches only the build system c
...
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273161309)
> As an alternative approach, might it make more sense to use [`pycache_prefix`](https://docs.python.org/3/library/sys.html#sys.pycache_prefix) so we can address this concern globally instead of just for this file, and in the build system code instead of leaking this into non-build system code? And that way we can keep the (in this case negligible) performance benefits of caching bytecode?
I did consider it. I have to admit that I failed to find a solution that touches only the build system c
...
💬 josibake commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2273166427)
Small nit: can you update the description with what you mention in https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-2202254574 , regarding #30509 ? I was surprised to see this PR in draft and it took me a bit of digging in the comments to learn why. It's more clear when reading the "process separation" issue, but I think it would also be helpful to mention in this PR description.
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2273166427)
Small nit: can you update the description with what you mention in https://github.com/bitcoin/bitcoin/pull/29409#pullrequestreview-2202254574 , regarding #30509 ? I was surprised to see this PR in draft and it took me a bit of digging in the comments to learn why. It's more clear when reading the "process separation" issue, but I think it would also be helpful to mention in this PR description.
💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#discussion_r1706787421)
Thanks! Updated.
(https://github.com/bitcoin/bitcoin/pull/30533#discussion_r1706787421)
Thanks! Updated.
💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273174597)
Addressed @stickies-v's https://github.com/bitcoin/bitcoin/pull/30533#discussion_r1706757028.
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273174597)
Addressed @stickies-v's https://github.com/bitcoin/bitcoin/pull/30533#discussion_r1706757028.
💬 maflcko commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2273177988)
Well, I wrote the code, but for clarity:
review ACK 6a020f019f709b595b68516bb3772b811a962e7 👌
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM
...
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2273177988)
Well, I wrote the code, but for clarity:
review ACK 6a020f019f709b595b68516bb3772b811a962e7 👌
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM
...
💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2273180341)
@ariard Honestly I'm inclined to think that warning isn't necessary anymore. 24.x was two years ago, and anyone crazy enough to rely on first-seen has had plenty of warning; I'm not actually aware of any merchants or services still accepting unconfirmed transactions.
The last one I was aware of was the Chivo Bitcoin ATMs in El Salvador... but they would accept unconfirmed transactions even with BIP-125 opt-in enabled, so that example isn't actually relevant to this change.
If anything, I t
...
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2273180341)
@ariard Honestly I'm inclined to think that warning isn't necessary anymore. 24.x was two years ago, and anyone crazy enough to rely on first-seen has had plenty of warning; I'm not actually aware of any merchants or services still accepting unconfirmed transactions.
The last one I was aware of was the Chivo Bitcoin ATMs in El Salvador... but they would accept unconfirmed transactions even with BIP-125 opt-in enabled, so that example isn't actually relevant to this change.
If anything, I t
...
💬 maflcko commented on pull request "assumeUTXO: snapshot load, update VERSION msg height":
(https://github.com/bitcoin/bitcoin/pull/30418#issuecomment-2273182090)
I agree with all the feedback by @fjahr from last month.
Are you still working on this? Maybe turn it into a draft in the meantime?
(https://github.com/bitcoin/bitcoin/pull/30418#issuecomment-2273182090)
I agree with all the feedback by @fjahr from last month.
Are you still working on this? Maybe turn it into a draft in the meantime?
✅ maflcko closed an issue: "Degraded performance of node's RPC estimatesmartfee after upgrade to V27.1"
(https://github.com/bitcoin/bitcoin/issues/30555)
(https://github.com/bitcoin/bitcoin/issues/30555)
💬 maflcko commented on issue "Degraded performance of node's RPC estimatesmartfee after upgrade to V27.1":
(https://github.com/bitcoin/bitcoin/issues/30555#issuecomment-2273195555)
Closing for now, but please add any more details below (or in a new issue), if you have them, or if someone else runs into the same problem and has more details.
(https://github.com/bitcoin/bitcoin/issues/30555#issuecomment-2273195555)
Closing for now, but please add any more details below (or in a new issue), if you have them, or if someone else runs into the same problem and has more details.
💬 josibake commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2273202157)
Coming back to this and added it to my list of things to review. Is there an upstream PR this one depends on that I should be reviewing first, or is there another reason the PR is in draft?
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2273202157)
Coming back to this and added it to my list of things to review. Is there an upstream PR this one depends on that I should be reviewing first, or is there another reason the PR is in draft?
👍 tdb3 approved a pull request: "docs: doc update for mempoolfullrbf default + log deprecation"
(https://github.com/bitcoin/bitcoin/pull/30594#pullrequestreview-2224267056)
ACK 1f93e3c360f3da38a02fe6c01551f424a2d63dc9
(https://github.com/bitcoin/bitcoin/pull/30594#pullrequestreview-2224267056)
ACK 1f93e3c360f3da38a02fe6c01551f424a2d63dc9
💬 stickies-v commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273229890)
> I failed to find a solution that touches only the build system code and works in all scenarios, including running functional tests individually.
I'm also not sure that's something we should be doing? Python programs creating pycache files is expected Python behaviour, and not something we control. Users who wish to do so can easily avoid this behaviour in a multitude of ways, the shortest of which is running `python -B rpcauth-test.py` or setting env vars. I think we should strive to make o
...
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273229890)
> I failed to find a solution that touches only the build system code and works in all scenarios, including running functional tests individually.
I'm also not sure that's something we should be doing? Python programs creating pycache files is expected Python behaviour, and not something we control. Users who wish to do so can easily avoid this behaviour in a multitude of ways, the shortest of which is running `python -B rpcauth-test.py` or setting env vars. I think we should strive to make o
...
💬 theStack commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273230354)
@maflcko: I think the simplest (and IMHO most obvious) solution would be to just verify the height as well, then all of the problems you described should be solved? E.g. with something like
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index d8c1c27aae..612bb960e5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5660,10 +5660,11 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
{
LOCK(::cs_main);
- if (!GetParams().AssumeutxoFo
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2273230354)
@maflcko: I think the simplest (and IMHO most obvious) solution would be to just verify the height as well, then all of the problems you described should be solved? E.g. with something like
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index d8c1c27aae..612bb960e5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -5660,10 +5660,11 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
{
LOCK(::cs_main);
- if (!GetParams().AssumeutxoFo
...
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706830350)
> Can you run this in valgrind?
Thanks for the context @maflcko! Valgrind is currently isn't really [compatible with ARM-based Macs](https://github.com/LouisBrunner/valgrind-macos/issues/56).
> Dereferencing a nullopt is UB
And we can't use `BOOST_CHECK_EQUAL(set_opts({"-assumevalid=0"}).assumed_valid_block, std::make_optional(uint256::ZERO))` since there's no standard way to display optionals, right?
Ok, in that case just consider the test fix validated, it failed before the fix, p
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706830350)
> Can you run this in valgrind?
Thanks for the context @maflcko! Valgrind is currently isn't really [compatible with ARM-based Macs](https://github.com/LouisBrunner/valgrind-macos/issues/56).
> Dereferencing a nullopt is UB
And we can't use `BOOST_CHECK_EQUAL(set_opts({"-assumevalid=0"}).assumed_valid_block, std::make_optional(uint256::ZERO))` since there's no standard way to display optionals, right?
Ok, in that case just consider the test fix validated, it failed before the fix, p
...