💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916483128)
Nit: Fix the indentation here?
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916483128)
Nit: Fix the indentation here?
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1918165052)
In commit af817ebb2af5870c082984e41731569c090ff3fc
Nit: Unneeded whitespace change.
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1918165052)
In commit af817ebb2af5870c082984e41731569c090ff3fc
Nit: Unneeded whitespace change.
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916603572)
Nit: Select `ACTIVE` and `LOCKED_IN` from the enum class name?
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916603572)
Nit: Select `ACTIVE` and `LOCKED_IN` from the enum class name?
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916546718)
Why did you choose `0` here and `2016` for the period?
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916546718)
Why did you choose `0` here and `2016` for the period?
💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1918190252)
In commit 49afd714b40115b15ea0ef146b48e9d7a403a596
Can you give some more motivation for splitting this out into its own header, especially since it seems to be introducing a new circular dependency? It also not immediately clear to me why a file called implementation only holds abstract declarations. From what I understand this is supposed to hold declarations that are only relevant for external use by tests.
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1918190252)
In commit 49afd714b40115b15ea0ef146b48e9d7a403a596
Can you give some more motivation for splitting this out into its own header, especially since it seems to be introducing a new circular dependency? It also not immediately clear to me why a file called implementation only holds abstract declarations. From what I understand this is supposed to hold declarations that are only relevant for external use by tests.
💬 l0rinc commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2595151977)
Since profilers may not catch these short-lived spikes, I've instrumented the code, loaded the UTXO set (as described the in the PR), parsed the logged flushing times and memory usages and plotted them against each other to see the effect of the batch size increase.
<details>
<summary>txdb.cpp flush time and memory instrumentation:</summary>
```patch
diff --git a/src/txdb.cpp b/src/txdb.cpp
--- a/src/txdb.cpp (revision d249a353be58868d41d2a7c57357038ffd779eba)
+++ b/src/txdb.cpp (revis
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2595151977)
Since profilers may not catch these short-lived spikes, I've instrumented the code, loaded the UTXO set (as described the in the PR), parsed the logged flushing times and memory usages and plotted them against each other to see the effect of the batch size increase.
<details>
<summary>txdb.cpp flush time and memory instrumentation:</summary>
```patch
diff --git a/src/txdb.cpp b/src/txdb.cpp
--- a/src/txdb.cpp (revision d249a353be58868d41d2a7c57357038ffd779eba)
+++ b/src/txdb.cpp (revis
...
👍 hebasto approved a pull request: "ci: Turn CentOS task into native one"
(https://github.com/bitcoin/bitcoin/pull/31651#pullrequestreview-2555513010)
ACK fabefd9915802d53d8c83ae66e5cb259196d9422, tested locally on Ubuntu 24.10.
(https://github.com/bitcoin/bitcoin/pull/31651#pullrequestreview-2555513010)
ACK fabefd9915802d53d8c83ae66e5cb259196d9422, tested locally on Ubuntu 24.10.
💬 hebasto commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#discussion_r1918212255)
Shouldn't "depends" be mentioned here for consistency with other task names?
(https://github.com/bitcoin/bitcoin/pull/31651#discussion_r1918212255)
Shouldn't "depends" be mentioned here for consistency with other task names?
💬 dergoegge commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1918215963)
I mostly added that assertion to prevent a buffer overflow in the `memcmp` below, which won't happen as long as the `program` span is 32 bytes in size or larger.
I can see the value in asserting that `program` is an actually a v0 script hash (i.e. 32 bytes exactly). I'll consider changing it if I retouch.
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1918215963)
I mostly added that assertion to prevent a buffer overflow in the `memcmp` below, which won't happen as long as the `program` span is 32 bytes in size or larger.
I can see the value in asserting that `program` is an actually a v0 script hash (i.e. 32 bytes exactly). I'll consider changing it if I retouch.
📝 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
...