📝 guspan-tanadi opened a pull request: "doc: navigate section links"
(https://github.com/bitcoin/bitcoin/pull/31670)
(https://github.com/bitcoin/bitcoin/pull/31670)
💬 Sjors commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2595191840)
Can you rebase and address @luke-jr's comment? I'm happy to retest after that.
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2595191840)
Can you rebase and address @luke-jr's comment? I'm happy to retest after that.
💬 hebasto commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2595195560)
> It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
I'm not arguing with this change, but could someone provide a quote from the C++20 standard that stipulates this? From my reading of the standard, it appears that only dereferencing causes UB in such cases.
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2595195560)
> It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
I'm not arguing with this change, but could someone provide a quote from the C++20 standard that stipulates this? From my reading of the standard, it appears that only dereferencing causes UB in such cases.
💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2595197587)
Can you update the PR description to have a list of pre-requisite PRs?
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2595197587)
Can you update the PR description to have a list of pre-requisite PRs?
💬 maflcko commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#discussion_r1918244245)
Thx, will add in the follow-up, which touches this anyway
(https://github.com/bitcoin/bitcoin/pull/31651#discussion_r1918244245)
Thx, will add in the follow-up, which touches this anyway
💬 stickies-v commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918252087)
note: from https://docs.python.org/3/library/sys.html#sys.executable, there are no guarantees that `sys.executable` returns a Python path
> A string giving the absolute path of the executable binary for the Python interpreter, on systems where this makes sense. If Python is unable to retrieve the real path to its executable, [sys.executable](https://docs.python.org/3/library/sys.html#sys.executable) will be an empty string or None.
It seems like this shouldn't be an issue in normal circums
...
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918252087)
note: from https://docs.python.org/3/library/sys.html#sys.executable, there are no guarantees that `sys.executable` returns a Python path
> A string giving the absolute path of the executable binary for the Python interpreter, on systems where this makes sense. If Python is unable to retrieve the real path to its executable, [sys.executable](https://docs.python.org/3/library/sys.html#sys.executable) will be an empty string or None.
It seems like this shouldn't be an issue in normal circums
...
💬 stickies-v commented on pull request "qa: Use `sys.executable` when invoking other Python scripts":
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918228589)
nit: these functions don't return paths. `mock_signer_command` would be more appropriate. Alternatively, they could keep returning paths, and prepending the python binary could be a responsibility of the callsite (potentially through ` py_run()` helper function.
(https://github.com/bitcoin/bitcoin/pull/31541#discussion_r1918228589)
nit: these functions don't return paths. `mock_signer_command` would be more appropriate. Alternatively, they could keep returning paths, and prepending the python binary could be a responsibility of the callsite (potentially through ` py_run()` helper function.
👍 stickies-v approved a pull request: "qa: Use `sys.executable` when invoking other Python scripts"
(https://github.com/bitcoin/bitcoin/pull/31541#pullrequestreview-2555555176)
ACK d38ade7bc409207bf425e05ee10b633b0aef4c36 . I have a minor concern about `sys.executable` not being guaranteed to return a valid Python path, but this patch seems good enough as is so no blocker.
The mock scripts (e.g. `mock/multi_signers.py`) are executed from within functional tests such as `wallet_signer.py`. The mocks contain a shebang (`#!/usr/bin/env python3`), but shebangs are not well supported on Windows, which is why prior to this commit, `py -3` was prefixed to the path.
Prio
...
(https://github.com/bitcoin/bitcoin/pull/31541#pullrequestreview-2555555176)
ACK d38ade7bc409207bf425e05ee10b633b0aef4c36 . I have a minor concern about `sys.executable` not being guaranteed to return a valid Python path, but this patch seems good enough as is so no blocker.
The mock scripts (e.g. `mock/multi_signers.py`) are executed from within functional tests such as `wallet_signer.py`. The mocks contain a shebang (`#!/usr/bin/env python3`), but shebangs are not well supported on Windows, which is why prior to this commit, `py -3` was prefixed to the path.
Prio
...
💬 Sjors commented on pull request "gui, psbt: Use SIGHASH_DEFAULT when signing PSBTs":
(https://github.com/bitcoin-core/gui/pull/850#issuecomment-2595225719)
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e
It would be useful for the `FillPSBT` doc in `wallet.h` to explain that passing `SIGHASH_DEFAULT` will use `SIGHASH_ALL` for pre-taproot inputs.
You have to crawl all the way to `MutableTransactionSignatureCreator::CreateSig` to find this out:
```cpp
// BASE/WITNESS_V0 signatures don't support explicit SIGHASH_DEFAULT, use SIGHASH_ALL instead.
const int hashtype = nHashType == SIGHASH_DEFAULT ? SIGHASH_ALL : nHashType;
```
(https://github.com/bitcoin-core/gui/pull/850#issuecomment-2595225719)
utACK 3e97ff9c5eaa3160426ba112930b047404c54c9e
It would be useful for the `FillPSBT` doc in `wallet.h` to explain that passing `SIGHASH_DEFAULT` will use `SIGHASH_ALL` for pre-taproot inputs.
You have to crawl all the way to `MutableTransactionSignatureCreator::CreateSig` to find this out:
```cpp
// BASE/WITNESS_V0 signatures don't support explicit SIGHASH_DEFAULT, use SIGHASH_ALL instead.
const int hashtype = nHashType == SIGHASH_DEFAULT ? SIGHASH_ALL : nHashType;
```
📝 fanquake opened a pull request: "Update leveldb subtree to latest upstream"
(https://github.com/bitcoin/bitcoin/pull/31671)
Includes:
* https://github.com/bitcoin-core/leveldb-subtree/pull/40 (used in #29852)
* https://github.com/bitcoin-core/leveldb-subtree/pull/45
* https://github.com/bitcoin-core/leveldb-subtree/pull/46
(https://github.com/bitcoin/bitcoin/pull/31671)
Includes:
* https://github.com/bitcoin-core/leveldb-subtree/pull/40 (used in #29852)
* https://github.com/bitcoin-core/leveldb-subtree/pull/45
* https://github.com/bitcoin-core/leveldb-subtree/pull/46
💬 maflcko commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2595242417)
> > It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
>
> I'm not arguing with this change, but could someone provide a quote from the C++20 standard that stipulates this? From my reading of the standard, it appears that only dereferencing causes UB in such cases.
No, the addition explicitly declares this as UB, see https://eel.is/c++draft/expr.add#4:
> When an expression J that has integral type is a
...
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2595242417)
> > It is UB to apply a distance to a pointer or iterator further than the end itself, even if the distance is (partially) revoked later on.
>
> I'm not arguing with this change, but could someone provide a quote from the C++20 standard that stipulates this? From my reading of the standard, it appears that only dereferencing causes UB in such cases.
No, the addition explicitly declares this as UB, see https://eel.is/c++draft/expr.add#4:
> When an expression J that has integral type is a
...
💬 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.