💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752254628)
nit: I think this would be the same, just shorter?
```suggestion
if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return !(blockindex->nStatus & BLOCK_HAVE_MASK)) ||
```
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752254628)
nit: I think this would be the same, just shorter?
```suggestion
if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return !(blockindex->nStatus & BLOCK_HAVE_MASK)) ||
```
💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752259940)
Also it seems like the `!blockindex` can be taken out safely since it's checked a few lines above and if the check fails we throw an error.
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752259940)
Also it seems like the `!blockindex` can be taken out safely since it's checked a few lines above and if the check fails we throw an error.
💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752270427)
> I think the suggestion doesn't work because we still need to feed the result into have_undo so that if UndoReadFromDisk fails for an unexpected reason like disk errors, we don't attempt to access blockUndo below and segfault.
I see, I think in that scenario it would be best to throw an error right there instead of ignoring it silently and giving the user an incomplete response? I mean, we have all indication that we should have the undo data in that scenario and then we can not read it, it
...
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752270427)
> I think the suggestion doesn't work because we still need to feed the result into have_undo so that if UndoReadFromDisk fails for an unexpected reason like disk errors, we don't attempt to access blockUndo below and segfault.
I see, I think in that scenario it would be best to throw an error right there instead of ignoring it silently and giving the user an incomplete response? I mean, we have all indication that we should have the undo data in that scenario and then we can not read it, it
...
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2341427105)
I'm going to hold off reviewing this until I see the TxGraph PR to better understand its intended usage. :+1:
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2341427105)
I'm going to hold off reviewing this until I see the TxGraph PR to better understand its intended usage. :+1:
⚠️ mcelrath opened an issue: "Race condition between ZMQ UpdateTip and getblocktemplate"
(https://github.com/bitcoin/bitcoin/issues/30862)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When ZMQ announces a new chain tip via "sequence" "C" message, if `getblocktemplate` is called immediately, it can return a block template that does not reflect the new chain tip.
This is important for Braidpool, which needs to switch to a new chain tip as quickly as possible when a new block comes in.
### Expected behaviour
`getblocktemplate` should return a result consistent with th
...
(https://github.com/bitcoin/bitcoin/issues/30862)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When ZMQ announces a new chain tip via "sequence" "C" message, if `getblocktemplate` is called immediately, it can return a block template that does not reflect the new chain tip.
This is important for Braidpool, which needs to switch to a new chain tip as quickly as possible when a new block comes in.
### Expected behaviour
`getblocktemplate` should return a result consistent with th
...
💬 ryanofsky commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2341480416)
Are there release notes for cmake transation? Seems like the autoconf -> cmake mapping table https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e would be especially helpful to have in release notes to help users and packagers convert previous configurations to new ones.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2341480416)
Are there release notes for cmake transation? Seems like the autoconf -> cmake mapping table https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e would be especially helpful to have in release notes to help users and packagers convert previous configurations to new ones.
🤔 stickies-v reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2291996189)
I'm still a strong concept ACK on this, but I wonder if the incremental appoach currently taken is the best approach. We'll have to (I think quite likely non-trivially) update the logic based on flags, widths, precisions, conversion, ... actually used in the codebase, as seen with e.g. the precision and zero-padding comments?
So perhaps it'd be sensible to first create a branch that forces `ConstevalFormatString` everywhere, informing our requirements and adding full unit test coverage, and t
...
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2291996189)
I'm still a strong concept ACK on this, but I wonder if the incremental appoach currently taken is the best approach. We'll have to (I think quite likely non-trivially) update the logic based on flags, widths, precisions, conversion, ... actually used in the codebase, as seen with e.g. the precision and zero-padding comments?
So perhaps it'd be sensible to first create a branch that forces `ConstevalFormatString` everywhere, informing our requirements and adding full unit test coverage, and t
...
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751671633)
Note: it seems we are explicitly preventing `*` dynamic width/precision specifiers here, which was previously allowed (and checked) with the linter. That's not necessarily bad, it seems we're not currently using them (quick non-exhaustive search) and it can of course be added back in later, and we're throwing instead of silently ignoring it.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751671633)
Note: it seems we are explicitly preventing `*` dynamic width/precision specifiers here, which was previously allowed (and checked) with the linter. That's not necessarily bad, it seems we're not currently using them (quick non-exhaustive search) and it can of course be added back in later, and we're throwing instead of silently ignoring it.
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752314431)
We do have quite a few use cases of `\.[\d+]f` precision that would throw here. No issue in the current code, but would like have to be overhauled in the next PR?
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752314431)
We do have quite a few use cases of `\.[\d+]f` precision that would throw here. No issue in the current code, but would like have to be overhauled in the next PR?
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751669308)
I think having a separate `CountNumberFormatSpecifiers` would have made testing this function a bit easier and with better error feedback, while simultaneously also reducing template instantiation. Neither are overwhelmingly strong arguments, but I don't think there are really any downsides either?
E.g. sample, pretty rough (but passes tests) diff:
<details>
<summary>git diff on faa32adbcf</summary>
```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751669308)
I think having a separate `CountNumberFormatSpecifiers` would have made testing this function a bit easier and with better error feedback, while simultaneously also reducing template instantiation. Neither are overwhelmingly strong arguments, but I don't think there are really any downsides either?
E.g. sample, pretty rough (but passes tests) diff:
<details>
<summary>git diff on faa32adbcf</summary>
```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
...
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2341522716)
@instagibbs Sure.
To give a bit more background, my idea to store things in `Cluster` form and convert to `DepGraph`, was due to a (mistaken) belief that incremental changes to a `DepGraph` would be $\mathcal{O}(n)$ per-dependency, meaning $\mathcal{O(n^2)$ per-transaction, and thus potentially $\mathcal{O}(n^3)$ for a full $n$ transactions.
@sdaftuar pointed out the possibility of an $\mathcal{O(n)}$ per-transaction algorithm, or $\mathcal{O(n^2)}$ for $n$ transactions, implemented in thi
...
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2341522716)
@instagibbs Sure.
To give a bit more background, my idea to store things in `Cluster` form and convert to `DepGraph`, was due to a (mistaken) belief that incremental changes to a `DepGraph` would be $\mathcal{O}(n)$ per-dependency, meaning $\mathcal{O(n^2)$ per-transaction, and thus potentially $\mathcal{O}(n^3)$ for a full $n$ transactions.
@sdaftuar pointed out the possibility of an $\mathcal{O(n)}$ per-transaction algorithm, or $\mathcal{O(n^2)}$ for $n$ transactions, implemented in thi
...
✅ ryanofsky closed an issue: "Improve minimum file descriptor accounting and documentation"
(https://github.com/bitcoin/bitcoin/issues/18911)
(https://github.com/bitcoin/bitcoin/issues/18911)
🚀 ryanofsky merged a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065)
(https://github.com/bitcoin/bitcoin/pull/30065)
⚠️ dergoegge opened an issue: "fuzz: `scriptpubkeyman`: heap-buffer-overflow miniscript.cpp in CScript BuildScript"
(https://github.com/bitcoin/bitcoin/issues/30864)
```
$ echo "dHIoJTE3LzwyOzM+LGw6cGsoJTA4KSk=" | base64 --decode > scriptpubkeyman.crash
$ FUZZ=scriptpubkeyman src/test/fuzz/fuzz scriptpubkeyman.crash
...
SUMMARY: AddressSanitizer: heap-buffer-overflow miniscript.cpp in CScript BuildScript<opcodetype, CScript&, opcodetype, CScript&, opcodetype>(opcodetype&&, CScript&, opcodetype&&, CScript&, opcodetype&&)
...
```
(https://github.com/bitcoin/bitcoin/issues/30864)
```
$ echo "dHIoJTE3LzwyOzM+LGw6cGsoJTA4KSk=" | base64 --decode > scriptpubkeyman.crash
$ FUZZ=scriptpubkeyman src/test/fuzz/fuzz scriptpubkeyman.crash
...
SUMMARY: AddressSanitizer: heap-buffer-overflow miniscript.cpp in CScript BuildScript<opcodetype, CScript&, opcodetype, CScript&, opcodetype>(opcodetype&&, CScript&, opcodetype&&, CScript&, opcodetype&&)
...
```
💬 QBlockQ commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2341564709)
Just a suggestion (you may send me a BIG thanks after a decade), why don't you consider integration of post-quantum cryptography (PQC) in this time for the next release of Bitcoin Core 27.1. PQC integration code is ready!
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2341564709)
Just a suggestion (you may send me a BIG thanks after a decade), why don't you consider integration of post-quantum cryptography (PQC) in this time for the next release of Bitcoin Core 27.1. PQC integration code is ready!
💬 achow101 commented on issue "fuzz: `scriptpubkeyman`: heap-buffer-overflow miniscript.cpp in CScript BuildScript":
(https://github.com/bitcoin/bitcoin/issues/30864#issuecomment-2341582360)
I believe this is only an issue with the fuzzer as I can't trigger this crash outside of it. However, it does reveal an actual issue in the handling of multipath key expressions with miniscript.
The issue appears to be because `MiniscriptDescriptor`'s `m_node` is shallow copied, and when cloned fragments belonging to the multipath components are destroyed later, various shared_ptrs end up also being destroyed.
The following diff fixes this particular crash, but I think it is insufficient:
...
(https://github.com/bitcoin/bitcoin/issues/30864#issuecomment-2341582360)
I believe this is only an issue with the fuzzer as I can't trigger this crash outside of it. However, it does reveal an actual issue in the handling of multipath key expressions with miniscript.
The issue appears to be because `MiniscriptDescriptor`'s `m_node` is shallow copied, and when cloned fragments belonging to the multipath components are destroyed later, various shared_ptrs end up also being destroyed.
The following diff fixes this particular crash, but I think it is insufficient:
...
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752469255)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752469255)
Thanks, fixed.
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752472646)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752472646)
Thanks, fixed.
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1752485188)
This error was reported [many](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107852) [times](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109569) for GCC 12.2, managed to reproduce the error via [godbolt](https://godbolt.org/z/8r9TKKoxv) (it's fixed in GCC 12.3) and on a local Dockerfile:
<details>
<summary>Dockerfile</summary>
```dockerfile
FROM docker.io/amd64/debian:bookworm
# Update and install dependencies
RUN apt update && apt install -y \
build-essential \
cmake \
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1752485188)
This error was reported [many](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107852) [times](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109569) for GCC 12.2, managed to reproduce the error via [godbolt](https://godbolt.org/z/8r9TKKoxv) (it's fixed in GCC 12.3) and on a local Dockerfile:
<details>
<summary>Dockerfile</summary>
```dockerfile
FROM docker.io/amd64/debian:bookworm
# Update and install dependencies
RUN apt update && apt install -y \
build-essential \
cmake \
...
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752491985)
> Sorry for not realizing and raising this in an earlier review.
No worries. It was my mistake for pushing the wrong diff.
My initial patch from July only had very basic checking and accepted any format specifier (like tinyformat). When I added support for positional args a few days ago (https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098), I also tried to validate format specifiers itself.
However, given that tinyformat accepts any specifier, and given that incorrec
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752491985)
> Sorry for not realizing and raising this in an earlier review.
No worries. It was my mistake for pushing the wrong diff.
My initial patch from July only had very basic checking and accepted any format specifier (like tinyformat). When I added support for positional args a few days ago (https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098), I also tried to validate format specifiers itself.
However, given that tinyformat accepts any specifier, and given that incorrec
...