📝 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.
🤔 l0rinc reviewed a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-2854173281)
Thanks for the review!
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-2854173281)
Thanks for the review!
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097976720)
we could either do `size_t{1}` or `leaves.reserve(block.vtx.size() + (block.vtx.size() & 1));`, I think, if you think this could be problematic on 32 bit systems.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097976720)
we could either do `size_t{1}` or `leaves.reserve(block.vtx.size() + (block.vtx.size() & 1));`, I think, if you think this could be problematic on 32 bit systems.
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097969142)
After this change all call-sites are pre-sized to even number of leaves, so this reflects the usage better.
I could also round to `9002` to mimic the new `leaves.reserve((9001 + 1) & ~1ULL)` behavior, if you thinks that's more representative.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097969142)
After this change all call-sites are pre-sized to even number of leaves, so this reflects the usage better.
I could also round to `9002` to mimic the new `leaves.reserve((9001 + 1) & ~1ULL)` behavior, if you thinks that's more representative.
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097962172)
the `ComputeMerkleRoot` calculation has to be performed to be able to validate this assert - it's can't eliminate the call.
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2097962172)
the `ComputeMerkleRoot` calculation has to be performed to be able to validate this assert - it's can't eliminate the call.
💬 fanquake commented on pull request "depends: use "mkdir -p" when installing xproto":
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894417836)
It's documented in the install script.
(https://github.com/bitcoin/bitcoin/pull/32568#issuecomment-2894417836)
It's documented in the install script.
💬 hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2097984555)
This part seems too invasive to me. Please compare the list of environment variables in the "Get tool information" step on the master branch with those in this PR.
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2097984555)
This part seems too invasive to me. Please compare the list of environment variables in the "Get tool information" step on the master branch with those in this PR.
💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2098002051)
Is this different from your previous suggestion in https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1748973642?
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2098002051)
Is this different from your previous suggestion in https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1748973642?
🤔 vasild reviewed a pull request: "tests: Expand HTTP coverage to assert libevent behavior"
(https://github.com/bitcoin/bitcoin/pull/32408#pullrequestreview-2854214850)
Approach ACK 95d47449b2c069fca9859d76b2c3766868b5ec79
Copied my review comments from https://github.com/bitcoin/bitcoin/pull/32061
(https://github.com/bitcoin/bitcoin/pull/32408#pullrequestreview-2854214850)
Approach ACK 95d47449b2c069fca9859d76b2c3766868b5ec79
Copied my review comments from https://github.com/bitcoin/bitcoin/pull/32061
💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2097991881)
_Repeat comment https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090504818 here:_
1 second timeout to send or receive seems more than enough for local testing on a dev machine. However, CI virtual machines sometimes are surprisingly slow. To avoid unnecessary test failures maybe it would be better to have this be 5 or 10 seconds for the `sendall()` calls and then set to 1 for the `recv()` call which we expect to timeout.
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2097991881)
_Repeat comment https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2090504818 here:_
1 second timeout to send or receive seems more than enough for local testing on a dev machine. However, CI virtual machines sometimes are surprisingly slow. To avoid unnecessary test failures maybe it would be better to have this be 5 or 10 seconds for the `sendall()` calls and then set to 1 for the `recv()` call which we expect to timeout.