💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2097414194)
nit: Would be nice if `WriteUTXOSnapshot()` took `AutoFile&& afile` to signify a transfer of ownership.
<details><summary>diff</summary>
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 0fa66eb211..0acd8c3e98 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 0fa66eb211..0acd8c3e98 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -83,7 +83,7 @@ UniValue Wri
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2097414194)
nit: Would be nice if `WriteUTXOSnapshot()` took `AutoFile&& afile` to signify a transfer of ownership.
<details><summary>diff</summary>
```diff
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 0fa66eb211..0acd8c3e98 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 0fa66eb211..0acd8c3e98 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -83,7 +83,7 @@ UniValue Wri
...
🤔 luke-jr reviewed a pull request: "RPC: removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/pull/32501#pullrequestreview-2853851501)
Concept NACK, but open to being convinced otherwise. Not seeing any benefit to this over batching. At a minimum, it should keep backward compatibility.
(https://github.com/bitcoin/bitcoin/pull/32501#pullrequestreview-2853851501)
Concept NACK, but open to being convinced otherwise. Not seeing any benefit to this over batching. At a minimum, it should keep backward compatibility.
✅ maflcko closed a pull request: "RPC: removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/pull/32501)
(https://github.com/bitcoin/bitcoin/pull/32501)
💬 maflcko commented on pull request "RPC: removeprunedfunds should take an array of txids":
(https://github.com/bitcoin/bitcoin/pull/32501#issuecomment-2894196322)
Closing for now. I don't think there is any need in opening a pull request that is just a previously closed pull request with:
* the original author stripped,
* no reference to the previously closed pull,
* failing tests,
* no feedback addressed.
Contributions are welcome. Just make sure to run the tests before opening a pull request. Also, pre-existing review feedback would be good to address before opening a pull request.
(https://github.com/bitcoin/bitcoin/pull/32501#issuecomment-2894196322)
Closing for now. I don't think there is any need in opening a pull request that is just a previously closed pull request with:
* the original author stripped,
* no reference to the previously closed pull,
* failing tests,
* no feedback addressed.
Contributions are welcome. Just make sure to run the tests before opening a pull request. Also, pre-existing review feedback would be good to address before opening a pull request.
💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2894231304)
Rebased and taken @davidgumberg's great suggestion.
(https://github.com/bitcoin/bitcoin/pull/32513#issuecomment-2894231304)
Rebased and taken @davidgumberg's great suggestion.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097845078)
Done, thanks
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097845078)
Done, thanks
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097865341)
`xor_bytes_reference` randomly generates where the next offset will be, check it by adding:
```C++
void operator()(std::span<std::byte> target, const size_t key_offset_bytes = 0) const
{
if (!*this) return;
const uint64_t rot_key{m_rotations[key_offset_bytes % SIZE_BYTES]}; // Continue obfuscation from where we left off
std::cout << "key_offset_bytes % SIZE_BYTES = " << (key_offset_bytes % SIZE_BYTES) << std::endl;
```
which reveals:
```
key_offset_bytes
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097865341)
`xor_bytes_reference` randomly generates where the next offset will be, check it by adding:
```C++
void operator()(std::span<std::byte> target, const size_t key_offset_bytes = 0) const
{
if (!*this) return;
const uint64_t rot_key{m_rotations[key_offset_bytes % SIZE_BYTES]}; // Continue obfuscation from where we left off
std::cout << "key_offset_bytes % SIZE_BYTES = " << (key_offset_bytes % SIZE_BYTES) << std::endl;
```
which reveals:
```
key_offset_bytes
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097879589)
I don't think so, if this isn't true, we're in trouble - so it has to be the case in prod as well.
The godbold reproducer indicates this line will be eliminated in prod if all calls are demonstrably so already.
I think I had a bug here at one point so added this line to make sure it's easily detectable.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097879589)
I don't think so, if this isn't true, we're in trouble - so it has to be the case in prod as well.
The godbold reproducer indicates this line will be eliminated in prod if all calls are demonstrably so already.
I think I had a bug here at one point so added this line to make sure it's easily detectable.
💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2894299769)
What I don't understand is why the CentOS task seemingly passed before, but started failing after https://github.com/bitcoin/bitcoin/commit/af65fd1a333011137dafd3df9a51704fd319feb4, even though the task wasn't modified apart from a comment.
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2894299769)
What I don't understand is why the CentOS task seemingly passed before, but started failing after https://github.com/bitcoin/bitcoin/commit/af65fd1a333011137dafd3df9a51704fd319feb4, even though the task wasn't modified apart from a comment.
📝 maflcko reopened a pull request: "rev_32343_wip_nomerge_ci"
(https://github.com/bitcoin/bitcoin/pull/32535)
pls ignore this is just a #32524 playground
(https://github.com/bitcoin/bitcoin/pull/32535)
pls ignore this is just a #32524 playground
💬 maflcko commented on pull request "rev_32343_wip_nomerge_ci":
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2894301545)
(take 2 :sob: )
(https://github.com/bitcoin/bitcoin/pull/32535#issuecomment-2894301545)
(take 2 :sob: )
🤔 hebasto reviewed a pull request: "depends: use "mkdir -p" when installing xproto"
(https://github.com/bitcoin/bitcoin/pull/32568#pullrequestreview-2854071175)
> It looks like the mkdir detection in xproto is broken on Alpine.
Yeah, it seems [so](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15137882165/job/42553939523):
```
checking for a thread-safe mkdir -p... ./install-sh -c -d
```
In that case, why not guide all build scripts with `$($(package)_autoconf) MKDIR_P="mkdir -p"`?
(https://github.com/bitcoin/bitcoin/pull/32568#pullrequestreview-2854071175)
> It looks like the mkdir detection in xproto is broken on Alpine.
Yeah, it seems [so](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15137882165/job/42553939523):
```
checking for a thread-safe mkdir -p... ./install-sh -c -d
```
In that case, why not guide all build scripts with `$($(package)_autoconf) MKDIR_P="mkdir -p"`?
💬 fanquake commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894311549)
> In that case, why not guide all build scripts with $($(package)_autoconf) MKDIR_P="mkdir -p"?
Not sure what you mean, but I'm not planning on changing things globally to work around a single broken package on a single OS.
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894311549)
> In that case, why not guide all build scripts with $($(package)_autoconf) MKDIR_P="mkdir -p"?
Not sure what you mean, but I'm not planning on changing things globally to work around a single broken package on a single OS.
💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097903049)
I wanted to avoid long-range estimates based on block height, due to `dTxRate` being based on time (number per second), but I guess everything is an estimate anyway and `dTxRate` could be changed to be a number of txs per block if it mattered.
However, I don't see the benefit of your suggestion. If miner-set timestamps are off long-range, this debug stat is the least thing to worry about. Also, the fallback time-based code will have to be kept for IBD-catch-up scenarios.
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2097903049)
I wanted to avoid long-range estimates based on block height, due to `dTxRate` being based on time (number per second), but I guess everything is an estimate anyway and `dTxRate` could be changed to be a number of txs per block if it mattered.
However, I don't see the benefit of your suggestion. If miner-set timestamps are off long-range, this debug stat is the least thing to worry about. Also, the fallback time-based code will have to be kept for IBD-catch-up scenarios.
💬 hebasto commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894330387)
> > In that case, why not guide all build scripts with $($(package)_autoconf) MKDIR_P="mkdir -p"?
>
> Not sure what you mean, but I'm not planning on changing things globally...
I do not mean nothing like that, but to configure the `xproto` package's build system with the [documented](https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/html_node/Particular-Programs.html) variable.
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894330387)
> > In that case, why not guide all build scripts with $($(package)_autoconf) MKDIR_P="mkdir -p"?
>
> Not sure what you mean, but I'm not planning on changing things globally...
I do not mean nothing like that, but to configure the `xproto` package's build system with the [documented](https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.72/html_node/Particular-Programs.html) variable.
💬 fanquake commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894338249)
`MKDIR_P` is also documented. What's the difference?
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894338249)
`MKDIR_P` is also documented. What's the difference?
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097927895)
I'm not sure about the alignment part, seems dangerous - do you see an explanation in the assembly why it would be faster?
I tried reproducing it, but I'm getting a significant slowdown instead (ran each 3x):
<details>
<summary>Current loop:</summary>
```C++
void operator()(std::span<std::byte> target, const size_t key_offset_bytes = 0) const
{
if (!*this) return;
const uint64_t rot_key{m_rotations[key_offset_bytes % SIZE_BYTES]}; // Continue obfuscation from where we left of
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2097927895)
I'm not sure about the alignment part, seems dangerous - do you see an explanation in the assembly why it would be faster?
I tried reproducing it, but I'm getting a significant slowdown instead (ran each 3x):
<details>
<summary>Current loop:</summary>
```C++
void operator()(std::span<std::byte> target, const size_t key_offset_bytes = 0) const
{
if (!*this) return;
const uint64_t rot_key{m_rotations[key_offset_bytes % SIZE_BYTES]}; // Continue obfuscation from where we left of
...
📝 l0rinc converted_to_draft a pull request: "script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532)
### Summary
For the vast majority of scripts we encounter, the sig-op count can be deduced directly from their fixed-length template (e.g. P2PKH, P2SH, P2WPKH, P2WSH, P2TR …), without iterating the script manually.
### Context
While profiling the remaining **SwiftSync** bottlenecks, I saw that [Own Samples](https://youtrack.jetbrains.com/issue/CPP-30003) highlighted `GetSigOpCount` as a non-negligible slice of validation time.
Test coverage for this code path was also thin, and 3 recen
...
(https://github.com/bitcoin/bitcoin/pull/32532)
### Summary
For the vast majority of scripts we encounter, the sig-op count can be deduced directly from their fixed-length template (e.g. P2PKH, P2SH, P2WPKH, P2WSH, P2TR …), without iterating the script manually.
### Context
While profiling the remaining **SwiftSync** bottlenecks, I saw that [Own Samples](https://youtrack.jetbrains.com/issue/CPP-30003) highlighted `GetSigOpCount` as a non-negligible slice of validation time.
Test coverage for this code path was also thin, and 3 recen
...
💬 hebasto commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894395724)
> `MKDIRPROG` is also documented.
Mind sharing a link please?
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894395724)
> `MKDIRPROG` is also documented.
Mind sharing a link please?
👍 hebasto approved a pull request: "fs: remove `_POSIX_C_SOURCE` defining"
(https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2854183748)
ACK db895c0e38427ce73167ef325f5731a3c4805598.
(https://github.com/bitcoin/bitcoin/pull/32460#pullrequestreview-2854183748)
ACK db895c0e38427ce73167ef325f5731a3c4805598.