💬 l0rinc commented on pull request "leveldb: show non-default options during init":
(https://github.com/bitcoin/bitcoin/pull/31644#issuecomment-2595253438)
> How is this useful?
I've extended the description with more details.
<details>
<summary>Details</summary>
```bash
2025-01-16T10:47:35Z Opening LevelDB in /Users/lorinc/Library/Application Support/Bitcoin/blocks/index
2025-01-16T10:47:35Z Opened LevelDB successfully with options: options.compression=NoCompression, options.create_if_missing=true, options.max_file_size=33554432, options.paranoid_checks=true, options.write_buffer_size=524288, readoptions.fill_cache=false, readoptions.ver
...
(https://github.com/bitcoin/bitcoin/pull/31644#issuecomment-2595253438)
> How is this useful?
I've extended the description with more details.
<details>
<summary>Details</summary>
```bash
2025-01-16T10:47:35Z Opening LevelDB in /Users/lorinc/Library/Application Support/Bitcoin/blocks/index
2025-01-16T10:47:35Z Opened LevelDB successfully with options: options.compression=NoCompression, options.create_if_missing=true, options.max_file_size=33554432, options.paranoid_checks=true, options.write_buffer_size=524288, readoptions.fill_cache=false, readoptions.ver
...
💬 maflcko commented on pull request "ci: change the build to be verbose by default":
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2595278407)
Yes, I understand they are not identical. As pointed out earlier, I don't see how this solves this class of issues: https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2578078036. It seems more like this is a one-off patch that was used once for debugging. I just don't see how it could have helped in that case (as opposed to using already existing info in logs). It would help to add exact steps to reproduce your use-case, otherwise, this all seems a bit speculative.
(https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2595278407)
Yes, I understand they are not identical. As pointed out earlier, I don't see how this solves this class of issues: https://github.com/bitcoin/bitcoin/pull/31619#issuecomment-2578078036. It seems more like this is a one-off patch that was used once for debugging. I just don't see how it could have helped in that case (as opposed to using already existing info in logs). It would help to add exact steps to reproduce your use-case, otherwise, this all seems a bit speculative.
💬 maflcko commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918298794)
> note: from https://docs.python.org/3/library/sys.html#sys.executable, there are no guarantees that `sys.executable` returns a Python path
For the purposes of functional test, it is guaranteed, because it is already used and enforced elsewhere. You can check this with `git grep 'sys.exec' test/functional/`.
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918298794)
> note: from https://docs.python.org/3/library/sys.html#sys.executable, there are no guarantees that `sys.executable` returns a Python path
For the purposes of functional test, it is guaranteed, because it is already used and enforced elsewhere. You can check this with `git grep 'sys.exec' test/functional/`.
💬 hebasto commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2595292934)
> This can be tested with a diff:
>
> ```diff
> diff --git a/src/crypto/sha3.cpp b/src/crypto/sha3.cpp
> index 770500bfe2..6eedba3ad6 100644
> --- a/src/crypto/sha3.cpp
> +++ b/src/crypto/sha3.cpp
> @@ -103,8 +103,10 @@ void KeccakF(uint64_t (&st)[25])
> }
> }
>
> -SHA3_256& SHA3_256::Write(Span<const unsigned char> data)
> +#include <span>
> +SHA3_256& SHA3_256::Write(Span<const unsigned char> old)
> {
> +std::span data{old};
> if (m_bufsize && m_bufsize + data.si
...
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2595292934)
> This can be tested with a diff:
>
> ```diff
> diff --git a/src/crypto/sha3.cpp b/src/crypto/sha3.cpp
> index 770500bfe2..6eedba3ad6 100644
> --- a/src/crypto/sha3.cpp
> +++ b/src/crypto/sha3.cpp
> @@ -103,8 +103,10 @@ void KeccakF(uint64_t (&st)[25])
> }
> }
>
> -SHA3_256& SHA3_256::Write(Span<const unsigned char> data)
> +#include <span>
> +SHA3_256& SHA3_256::Write(Span<const unsigned char> old)
> {
> +std::span data{old};
> if (m_bufsize && m_bufsize + data.si
...
💬 maflcko commented on pull request "doc: navigate section links":
(https://github.com/bitcoin/bitcoin/pull/31670#discussion_r1918302795)
The auto-generated anchor may depend on the markdown tool, so I guess this is optimizing for the one that github uses? Sadly there are no markdown anchors, which would solve this class of issues like in mediawiki.
(https://github.com/bitcoin/bitcoin/pull/31670#discussion_r1918302795)
The auto-generated anchor may depend on the markdown tool, so I guess this is optimizing for the one that github uses? Sadly there are no markdown anchors, which would solve this class of issues like in mediawiki.
💬 maflcko commented on pull request "doc: navigate section links":
(https://github.com/bitcoin/bitcoin/pull/31670#discussion_r1918301214)
I am halfway thinking this is a regression in the llvm upsteam repo. I would be better to fix (or report) it upstream.
(https://github.com/bitcoin/bitcoin/pull/31670#discussion_r1918301214)
I am halfway thinking this is a regression in the llvm upsteam repo. I would be better to fix (or report) it upstream.
💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#issuecomment-2595294790)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31495#issuecomment-2595294790)
Concept ACK
👍 hebasto approved a pull request: "refactor: Avoid UB in SHA3_256::Write"
(https://github.com/bitcoin/bitcoin/pull/31655#pullrequestreview-2555695646)
ACK fabeca3458b38a3d8930cb0cbc866388c3f120f1.
(https://github.com/bitcoin/bitcoin/pull/31655#pullrequestreview-2555695646)
ACK fabeca3458b38a3d8930cb0cbc866388c3f120f1.
💬 fanquake commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2595304334)
Assuming the CI is always configured to use `-DWERROR`, this seems like the more logical/generic solution for #31476 compared to #31669. Although one thing that could be improved is making the error more clear, as in the current output, it's hidden far back in the scrollback.
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2595304334)
Assuming the CI is always configured to use `-DWERROR`, this seems like the more logical/generic solution for #31476 compared to #31669. Although one thing that could be improved is making the error more clear, as in the current output, it's hidden far back in the scrollback.
💬 stickies-v commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918312609)
Ah yes, that's true, `test_runner` itself shouldn't even work in that case.
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918312609)
Ah yes, that's true, `test_runner` itself shouldn't even work in that case.
💬 fanquake commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2595310187)
> if we start using syntax features and behavior that are only available in versions greater than the minimum?
This should never be the case, otherwise the minimum required version isn't actually the minimum required version.
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2595310187)
> if we start using syntax features and behavior that are only available in versions greater than the minimum?
This should never be the case, otherwise the minimum required version isn't actually the minimum required version.
🤔 stickies-v reviewed a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2555656322)
Approach ACK c0127439597ee9d09b8e117be7d6226a68b45721
I'm not familiar enough with symlinks and build toolchains to have an opinion on c0127439597ee9d09b8e117be7d6226a68b45721, but it seems reasonable.
(https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2555656322)
Approach ACK c0127439597ee9d09b8e117be7d6226a68b45721
I'm not familiar enough with symlinks and build toolchains to have an opinion on c0127439597ee9d09b8e117be7d6226a68b45721, but it seems reasonable.
💬 stickies-v commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918282849)
Making this check conditional on platform is undocumented in the commit message, and seems orthogonal to the goal of testing errors individually. I think it would be best to split this out in a separate commit and document it?
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918282849)
Making this check conditional on platform is undocumented in the commit message, and seems orthogonal to the goal of testing errors individually. I think it would be best to split this out in a separate commit and document it?
💬 l0rinc commented on pull request "Update leveldb subtree to latest upstream":
(https://github.com/bitcoin/bitcoin/pull/31671#issuecomment-2595315830)
Could you share more about how these changes were selected. I noticed there have been many (useless) refactorings in LevelDB since the last stable version we’re using. While I completely agree that cherry-picking every update may not align with our goals (especially for many of the high-risk, low-reward refactors), a few targeted updates addressing critical issues might still be worth considering:
* [fb644cb](https://github.com/google/leveldb/commit/fb644cb44539925a7f444b1b0314f402a456c5f4): Fi
...
(https://github.com/bitcoin/bitcoin/pull/31671#issuecomment-2595315830)
Could you share more about how these changes were selected. I noticed there have been many (useless) refactorings in LevelDB since the last stable version we’re using. While I completely agree that cherry-picking every update may not align with our goals (especially for many of the high-risk, low-reward refactors), a few targeted updates addressing critical issues might still be worth considering:
* [fb644cb](https://github.com/google/leveldb/commit/fb644cb44539925a7f444b1b0314f402a456c5f4): Fi
...
💬 tdb3 commented on pull request "rpc: fix mintime field testnet4, expand timewarp attack test":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2595341267)
> I didn't know about #30941. Let me know if there's anything there that you'd like me to add here. After that I'll address the other feedback, in particular I should probably pick a higher number for `future` to better distinguish the wall clock scenario from the boundary condition check.
Since the test approach has changed, I'm not seeing more to add.
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2595341267)
> I didn't know about #30941. Let me know if there's anything there that you'd like me to add here. After that I'll address the other feedback, in particular I should probably pick a higher number for `future` to better distinguish the wall clock scenario from the boundary condition check.
Since the test approach has changed, I'm not seeing more to add.
💬 fanquake commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918336563)
In c0127439597ee9d09b8e117be7d6226a68b45721:
> because it depends on the build toolchain:
Even if this is the case, is the best option to just to do nothing? Shouldn't we atleast test the known behaviour for our release build, so we'd catch if this changes (is fixed?) in future? Otherwise my understand is if this changes again, it's just going to be hidden/ignored.
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918336563)
In c0127439597ee9d09b8e117be7d6226a68b45721:
> because it depends on the build toolchain:
Even if this is the case, is the best option to just to do nothing? Shouldn't we atleast test the known behaviour for our release build, so we'd catch if this changes (is fixed?) in future? Otherwise my understand is if this changes again, it's just going to be hidden/ignored.
💬 maflcko commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918354618)
I guess the check could be limited to exclude the msvc build?
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1918354618)
I guess the check could be limited to exclude the msvc build?
🚀 fanquake merged a pull request: "refactor: Avoid UB in SHA3_256::Write"
(https://github.com/bitcoin/bitcoin/pull/31655)
(https://github.com/bitcoin/bitcoin/pull/31655)
🤔 Sjors reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2555658830)
Concept ACK
The `descriptorprocesspsbt` documentation for `sighashtype` currently says:
> The signature hash type to sign with if not specified by the PSBT.
Let's change that to:
> The signature hash type to sign with. If the PSBT already specifies a hash type, it must match. Otherwise it is added to the PSBT.
Some comments based on partial code review.
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2555658830)
Concept ACK
The `descriptorprocesspsbt` documentation for `sighashtype` currently says:
> The signature hash type to sign with if not specified by the PSBT.
Let's change that to:
> The signature hash type to sign with. If the PSBT already specifies a hash type, it must match. Otherwise it is added to the PSBT.
Some comments based on partial code review.
💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1918339465)
b70e83da4210e8098cbf3fc3b2289fc710828455: in case of a hardware wallet (external signer) I would want this to fail before any call to the device is made.
It's a bit hard to tell if that's the case, maybe this specific check can be refactored into its own function? That would make things easier to follow in general.
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1918339465)
b70e83da4210e8098cbf3fc3b2289fc710828455: in case of a hardware wallet (external signer) I would want this to fail before any call to the device is made.
It's a bit hard to tell if that's the case, maybe this specific check can be refactored into its own function? That would make things easier to follow in general.