💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706662216)
> > has failed [000000016bcc2e20000000016bcc2e20000000016bcc29a03b66800104140f00 != 0000000000000000000000000000000000000000000000000000000000000000]
>
> Which I find a lot more revealing.
Can you run this in valgrind? Dereferencing a nullopt is UB (undefined behavior), which must be avoided in production (obviously), but also in tests.
If you want to learn which methods expose undefined behavior, I find [https://en.cppreference.com/w/cpp/utility/optional/operator*] a good resource.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706662216)
> > has failed [000000016bcc2e20000000016bcc2e20000000016bcc29a03b66800104140f00 != 0000000000000000000000000000000000000000000000000000000000000000]
>
> Which I find a lot more revealing.
Can you run this in valgrind? Dereferencing a nullopt is UB (undefined behavior), which must be avoided in production (obviously), but also in tests.
If you want to learn which methods expose undefined behavior, I find [https://en.cppreference.com/w/cpp/utility/optional/operator*] a good resource.
💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-2273008031)
Follow-up ideas for this pull requests are:
* The linearize scripts should be adjusted to apply any XOR, if it exists. C.f https://github.com/bitcoin/bitcoin/pull/28052/files#diff-cabb34205d48861dbb53b2ad0df92776bf7d917150caf17778e15fbc8e63e123R30
* A new test for undo data can be written: https://github.com/bitcoin/bitcoin/pull/28052/files#r1597462109
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-2273008031)
Follow-up ideas for this pull requests are:
* The linearize scripts should be adjusted to apply any XOR, if it exists. C.f https://github.com/bitcoin/bitcoin/pull/28052/files#diff-cabb34205d48861dbb53b2ad0df92776bf7d917150caf17778e15fbc8e63e123R30
* A new test for undo data can be written: https://github.com/bitcoin/bitcoin/pull/28052/files#r1597462109
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706674268)
That wouldn't work. `result_size` is used both to throw if the input is too large, as well as to pad with leading zeroes if it's too short: https://github.com/bitcoin/bitcoin/pull/30569/files#diff-3f688af8f182edecd9c33977b905b3e71dc010574f721fd4e328bdbc7706f574R59
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706674268)
That wouldn't work. `result_size` is used both to throw if the input is too large, as well as to pad with leading zeroes if it's too short: https://github.com/bitcoin/bitcoin/pull/30569/files#diff-3f688af8f182edecd9c33977b905b3e71dc010574f721fd4e328bdbc7706f574R59
💬 glozow commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677297)
done
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677297)
done
💬 glozow commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677230)
done
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677230)
done
💬 glozow commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677702)
right of course! done, thanks
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1706677702)
right of course! done, thanks
💬 maflcko commented on pull request "doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version":
(https://github.com/bitcoin/bitcoin/pull/30580#issuecomment-2273019342)
review-only ACK ed83974bb411ab5ebe3eef28f0ac995ce07936cd
(https://github.com/bitcoin/bitcoin/pull/30580#issuecomment-2273019342)
review-only ACK ed83974bb411ab5ebe3eef28f0ac995ce07936cd
💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706681205)
Thanks, will change to `int{x}` in next force push.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1706681205)
Thanks, will change to `int{x}` in next force push.
💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706688706)
Pretty sure there are two separate bugs here:
* The first is that `BOOST_NO_CXX98_FUNCTION_BASE` isn't removed from `CMAKE_REQUIRED_DEFINITIONS` after this check, which means it's incorrectly reused for the check below.
* The second is that when `-DWERROR=ON` is used, `-Werror` isn't being passed to the check for the Boost Test header (which also hides the first bug).
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706688706)
Pretty sure there are two separate bugs here:
* The first is that `BOOST_NO_CXX98_FUNCTION_BASE` isn't removed from `CMAKE_REQUIRED_DEFINITIONS` after this check, which means it's incorrectly reused for the check below.
* The second is that when `-DWERROR=ON` is used, `-Werror` isn't being passed to the check for the Boost Test header (which also hides the first bug).
💬 hebasto commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273059079)
Friendly ping @stickies-v @maflcko :)
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273059079)
Friendly ping @stickies-v @maflcko :)
💬 ajtowns commented on issue "RFC: ArgsManager type and range checking":
(https://github.com/bitcoin/bitcoin/issues/22978#issuecomment-2273070206)
> Posting to stop thinking about this for now, but I started working on a change that lets you declare type-safe settings without global constants, in a way that should be compatible with options structs and standalone setting retrieval.
Seems okay, though having a global type name that contains data (`UpnpSetting::Register` and `UpnpSetting::Update` have `summary.value` and `help.value`) doesn't seem meaningfully different to a global constant to me.
> This idea is to declare settings typ
...
(https://github.com/bitcoin/bitcoin/issues/22978#issuecomment-2273070206)
> Posting to stop thinking about this for now, but I started working on a change that lets you declare type-safe settings without global constants, in a way that should be compatible with options structs and standalone setting retrieval.
Seems okay, though having a global type name that contains data (`UpnpSetting::Register` and `UpnpSetting::Update` have `summary.value` and `help.value`) doesn't seem meaningfully different to a global constant to me.
> This idea is to declare settings typ
...
💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1706731123)
> I think a better approach is to introduce flags which are internally coherent and actually support features we know we will use, and then roll the flags slowly, to one setting at a time, changing behavior of each setting once rather than multiple times, and documenting changes explicitly.
I agree, but I don't think it is incompatible with my suggestion. It seems possible to just implement bool error checking (and only that, or any other single feature), and make sure it is coherent and will
...
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1706731123)
> I think a better approach is to introduce flags which are internally coherent and actually support features we know we will use, and then roll the flags slowly, to one setting at a time, changing behavior of each setting once rather than multiple times, and documenting changes explicitly.
I agree, but I don't think it is incompatible with my suggestion. It seems possible to just implement bool error checking (and only that, or any other single feature), and make sure it is coherent and will
...
💬 maflcko commented on pull request "test: Do not write Python bytecode to source directory":
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273096799)
CI is green and the diff looks reasonable, so I guess it is harmless and fine.
Did you try setting the source dir read-only, and it passes with this change?
(https://github.com/bitcoin/bitcoin/pull/30533#issuecomment-2273096799)
CI is green and the diff looks reasonable, so I guess it is harmless and fine.
Did you try setting the source dir read-only, and it passes with this change?
💬 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
...