📝 l0rinc opened a pull request: "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532)
### Draft — requesting feedback on the overall direction
---
### 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 scanning every opcode.
### 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.
...
(https://github.com/bitcoin/bitcoin/pull/32532)
### Draft — requesting feedback on the overall direction
---
### 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 scanning every opcode.
### 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.
...
💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
E.g on this branch
```
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/bitcoin ((a5ac43d9))> build/bin/bitcoin -h rpc
Usage: build/bin/bitcoin [OPTIONS] COMMAND...
Options:
-m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
-v, --version Show version information
-h, --help Show this help message
Commands:
gui [ARGS] Start GUI, equivale
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093116725)
E.g on this branch
```
abubakarismail@Abubakars-MacBook-Pro ~/D/W/b/bitcoin ((a5ac43d9))> build/bin/bitcoin -h rpc
Usage: build/bin/bitcoin [OPTIONS] COMMAND...
Options:
-m, --multiprocess Run multiprocess binaries bitcoin-node, bitcoin-gui.
-M, --monolithic Run monolithic binaries bitcoind, bitcoin-qt. (Default behavior)
-v, --version Show version information
-h, --help Show this help message
Commands:
gui [ARGS] Start GUI, equivale
...
💬 ismaelsadeeq commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093117141)
fair point.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2093117141)
fair point.
💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2886824545)
I have pushed the mentioned `SigOps` calculation optimization, adding the `/*fAccurate=*/` argument to each usage, and renamed `GetSigOpCount` to `GetLegacySigOpCount` to highlight how this differs from the post-segwit implementation.
I've also covered it with an extensive suite of tests and a benchmark, see: https://github.com/bitcoin/bitcoin/pull/32532
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2886824545)
I have pushed the mentioned `SigOps` calculation optimization, adding the `/*fAccurate=*/` argument to each usage, and renamed `GetSigOpCount` to `GetLegacySigOpCount` to highlight how this differs from the post-segwit implementation.
I've also covered it with an extensive suite of tests and a benchmark, see: https://github.com/bitcoin/bitcoin/pull/32532
💬 willcl-ark commented on issue "Depends toolchain doesn't contain enough info to build from depends on a fresh NixOS install":
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2886830462)
OK I updated that branch a little, it is now simplified, and works for Qt. However I did have to carve out Boost (bypassing the provider), for reasons that are not totally clear to me still.
(https://github.com/bitcoin/bitcoin/issues/32428#issuecomment-2886830462)
OK I updated that branch a little, it is now simplified, and works for Qt. However I did have to carve out Boost (bypassing the provider), for reasons that are not totally clear to me still.
💬 hebasto commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2886839571)
> > From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE
>
> `_GNU_SOURCE` isn't supposed to ever be defined by default, and I don't see us defining it anywhere...?
Do we use GNU extensions?
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2886839571)
> > From what I can tell, compilers on Linux systems, will be defining _GNU_SOURCE
>
> `_GNU_SOURCE` isn't supposed to ever be defined by default, and I don't see us defining it anywhere...?
Do we use GNU extensions?
💬 fanquake commented on pull request "[28.x] build: suppress `-Wunterminated-string-initialization` in secp256k1":
(https://github.com/bitcoin/bitcoin/pull/32484#issuecomment-2886842840)
Changed this, as the prior approach would warn under Clang.
(https://github.com/bitcoin/bitcoin/pull/32484#issuecomment-2886842840)
Changed this, as the prior approach would warn under Clang.
👍 pinheadmz approved a pull request: "rpc: Round verificationprogress to 1 for a recent tip"
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2846776698)
ACK 11116aa19f5173792f757276c8a58b279f18a199
Built and tested on macos/arm64. Reviewed the code. Grudgingly I accept that this is a user complaint that will never go away and since the PR only updates the UI I'm ok with it after all.
`guessVerificationProgress()` will now return `1` if the timestamp in the chain tip is within the last two hours. (Probably should have a release note?)
That gives the following result in the log when I finished syncing my mainnet full node (at about 13:40:
...
(https://github.com/bitcoin/bitcoin/pull/32528#pullrequestreview-2846776698)
ACK 11116aa19f5173792f757276c8a58b279f18a199
Built and tested on macos/arm64. Reviewed the code. Grudgingly I accept that this is a user complaint that will never go away and since the PR only updates the UI I'm ok with it after all.
`guessVerificationProgress()` will now return `1` if the timestamp in the chain tip is within the last two hours. (Probably should have a release note?)
That gives the following result in the log when I finished syncing my mainnet full node (at about 13:40:
...
💬 pinheadmz commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2093141438)
done
(https://github.com/bitcoin/bitcoin/pull/32517#discussion_r2093141438)
done
💬 l0rinc commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2886851641)
While sigops aren't necessarily difficult to compute, there's a lot of them - and now even more.
Please consider the related sigop optimization PR I just pushed: https://github.com/bitcoin/bitcoin/pull/32532
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-2886851641)
While sigops aren't necessarily difficult to compute, there's a lot of them - and now even more.
Please consider the related sigop optimization PR I just pushed: https://github.com/bitcoin/bitcoin/pull/32532
🤔 l0rinc requested changes to a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2846799717)
Can you please explain this in more detail? Why would a 64 bit system care about the 32 bit limits?
I'm already often setting the dbcache to values like 45GiB (which is already way over the 32 bit limit), I'm not sure I understand the motivation for capping the mempool size to below 4.5 GiB - see:
https://github.com/bitcoin/bitcoin/pull/31645/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R1092-R1097
Same for the surprisingly low default values, could you pleas
...
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2846799717)
Can you please explain this in more detail? Why would a 64 bit system care about the 32 bit limits?
I'm already often setting the dbcache to values like 45GiB (which is already way over the 32 bit limit), I'm not sure I understand the motivation for capping the mempool size to below 4.5 GiB - see:
https://github.com/bitcoin/bitcoin/pull/31645/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33R1092-R1097
Same for the surprisingly low default values, could you pleas
...
💬 l0rinc commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886877605)
Will this show `progress=1.000000` before the last block as well?
I find that more confusing than `0.999999` for the last one.
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886877605)
Will this show `progress=1.000000` before the last block as well?
I find that more confusing than `0.999999` for the last one.
💬 pinheadmz commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886905244)
> Will this show `progress=1.000000` before the last block as well?
How do you define "last block"? Or, more to the point, how does your full node "know" there are no more blocks out there? That's always been the UX issue, it's not accurate to ever claim we are 100% certain we are 100% synced.
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886905244)
> Will this show `progress=1.000000` before the last block as well?
How do you define "last block"? Or, more to the point, how does your full node "know" there are no more blocks out there? That's always been the UX issue, it's not accurate to ever claim we are 100% certain we are 100% synced.
💬 l0rinc commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886912645)
So what does the percentage mean if there's no fixed target value in the first place?
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886912645)
So what does the percentage mean if there's no fixed target value in the first place?
💬 pinheadmz commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886930544)
After this PR, it will mean your chain tip's timestamp is within 2 hours of now.
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2886930544)
After this PR, it will mean your chain tip's timestamp is within 2 hours of now.
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2092931817)
Passing `-1717429609` would make this code try to access `weekdays[-1]` :fire:
For modern style, better use `std::array` instead of C arrays and, more importantly, use the array's `at()` method which has boundary checks, just in case. Here is a change that adds some more tests and fixes the out-of-bounds access:
<details>
<summary>[patch] FormatRFC7231DateTime()</summary>
```diff
diff --git i/src/test/util_tests.cpp w/src/test/util_tests.cpp
index 387493152d..ebb40dd713 100644
---
...
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2092931817)
Passing `-1717429609` would make this code try to access `weekdays[-1]` :fire:
For modern style, better use `std::array` instead of C arrays and, more importantly, use the array's `at()` method which has boundary checks, just in case. Here is a change that adds some more tests and fixes the out-of-bounds access:
<details>
<summary>[patch] FormatRFC7231DateTime()</summary>
```diff
diff --git i/src/test/util_tests.cpp w/src/test/util_tests.cpp
index 387493152d..ebb40dd713 100644
---
...
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093076600)
e95c6f5b6511ae35141b1e440e1f22e1004d3de6
Copying strings is expensive:
```suggestion
std::optional<std::string_view> HTTPHeaders::Find(const std::string& key) const
```
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093076600)
e95c6f5b6511ae35141b1e440e1f22e1004d3de6
Copying strings is expensive:
```suggestion
std::optional<std::string_view> HTTPHeaders::Find(const std::string& key) const
```
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093078641)
```suggestion
void HTTPHeaders::Write(const std::string& key, const std::string& value)
```
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093078641)
```suggestion
void HTTPHeaders::Write(const std::string& key, const std::string& value)
```
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093181637)
Would be good to document when `Read()` throws, returns true and false.
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2093181637)
Would be good to document when `Read()` throws, returns true and false.
🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2846448811)
Posting review midway. Reviewed up to and including e95c6f5b6511ae35141b1e440e1f22e1004d3de6 `http: Implement HTTPHeaders class`
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2846448811)
Posting review midway. Reviewed up to and including e95c6f5b6511ae35141b1e440e1f22e1004d3de6 `http: Implement HTTPHeaders class`