💬 maflcko commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752268522)
> see: https://eel.is/c++draft/dcl.attr.assume
The assume attribute is C++23. However, this codebase is C++20 and this pull request is about the `Assume` macro in the codebase itself.
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752268522)
> see: https://eel.is/c++draft/dcl.attr.assume
The assume attribute is C++23. However, this codebase is C++20 and this pull request is about the `Assume` macro in the codebase itself.
💬 maflcko commented on pull request "wallet, rpc: (v31.0) remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-2752274763)
Please leave this in draft. 31.0 is half a year out, so starting review on this is questionable and won't help to get it merged earlier anyway
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-2752274763)
Please leave this in draft. 31.0 is half a year out, so starting review on this is questionable and won't help to get it merged earlier anyway
💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752276849)
> The assume attribute is C++23. However, this codebase is C++20 and this pull request is about the Assume macro in the codebase itself.
Good point. I did also link the C++23 link above. I'm just looking for confirmation that this doesn't appear in production which doesn't appear in any reference that I see.
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752276849)
> The assume attribute is C++23. However, this codebase is C++20 and this pull request is about the Assume macro in the codebase itself.
Good point. I did also link the C++23 link above. I'm just looking for confirmation that this doesn't appear in production which doesn't appear in any reference that I see.
📝 EniRox001 opened a pull request: "contrib: Add external RPC code generator with unit tests"
(https://github.com/bitcoin/bitcoin/pull/32140)
## Description
This pull request introduces an external tool for generating RPC client code based on the
Bitcoin Core RPC schema. The tool is implemented in Python using Jinja2 templates and can
generate code for both C++ and Python clients. In addition, unit tests have been added to
verify that the generator produces the expected output given a sample schema.
## Changes Included
- **`contrib/codegen/generate_client.py`**
A script that retrieves the latest RPC schema (via bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/32140)
## Description
This pull request introduces an external tool for generating RPC client code based on the
Bitcoin Core RPC schema. The tool is implemented in Python using Jinja2 templates and can
generate code for both C++ and Python clients. In addition, unit tests have been added to
verify that the generator produces the expected output given a sample schema.
## Changes Included
- **`contrib/codegen/generate_client.py`**
A script that retrieves the latest RPC schema (via bitcoi
...
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2752305824)
It occurs to me that it may be possible to get rid of `TxGraphImpl::Entry::m_main_lin_index`, by storing a transaction linearization position rather than its `DepGraphIndex` inside the `TxGraphImpl::Locator`. So instead of using the DepGraphIndex as primary way to identify a transaction within a cluster, make the linearization index the primary, and look up the other one through `m_linearization` when needed. It's not clear to me if it's better to leave `m_mapping` be indexed by DepGraphIndex or
...
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2752305824)
It occurs to me that it may be possible to get rid of `TxGraphImpl::Entry::m_main_lin_index`, by storing a transaction linearization position rather than its `DepGraphIndex` inside the `TxGraphImpl::Locator`. So instead of using the DepGraphIndex as primary way to identify a transaction within a cluster, make the linearization index the primary, and look up the other one through `m_linearization` when needed. It's not clear to me if it's better to leave `m_mapping` be indexed by DepGraphIndex or
...
💬 yancyribbens commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2012801346)
did you intend to leave this commented code?
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2012801346)
did you intend to leave this commented code?
📝 polespinasa converted_to_draft a pull request: "wallet, rpc: (v31.0) remove settxfee and paytxfee (work in progress)"
(https://github.com/bitcoin/bitcoin/pull/32138)
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
(https://github.com/bitcoin/bitcoin/pull/32138)
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
💬 ismaelsadeeq commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2012810735)
will fix not when there is need to retouch.
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2012810735)
will fix not when there is need to retouch.
💬 polespinasa commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2012813318)
yes, here the conversation https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2012813318)
yes, here the conversation https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768
💬 sipa commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752329360)
> I'm just looking for confirmation that this doesn't appear in production which doesn't appear in any reference that I see.
You cannot find any reference about it, because `Assume()` is a macro defined **in the Bitcoin Core codebase**, not a C++ language feature. It's defined [here](https://github.com/bitcoin/bitcoin/blob/v28.0/src/util/check.h#L89).
And you can't find confirmation that it doesn't appear in production, because as far as the language is concerned, it is. It just happens t
...
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2752329360)
> I'm just looking for confirmation that this doesn't appear in production which doesn't appear in any reference that I see.
You cannot find any reference about it, because `Assume()` is a macro defined **in the Bitcoin Core codebase**, not a C++ language feature. It's defined [here](https://github.com/bitcoin/bitcoin/blob/v28.0/src/util/check.h#L89).
And you can't find confirmation that it doesn't appear in production, because as far as the language is concerned, it is. It just happens t
...
👍 brunoerg approved a pull request: "fuzz: Fix off-by-one in package_rbf target"
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2714981577)
code review ACK fa5674c264d91eb3a99fa74ace8a1b6be113c0a8
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2714981577)
code review ACK fa5674c264d91eb3a99fa74ace8a1b6be113c0a8
💬 sipa commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2012818026)
This suggestion is not correct.
If the expression inside `Assume` has a side-effect, but still can be proven to evaluate to true, the side effect will still remain.
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2012818026)
This suggestion is not correct.
If the expression inside `Assume` has a side-effect, but still can be proven to evaluate to true, the side effect will still remain.
💬 hodlinator commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2752374109)
Since I introduced the [idea](https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827) to write the code like this, I'm happy to be part of resolving it. And happy to review your PR.
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2752374109)
Since I introduced the [idea](https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827) to write the code like this, I'm happy to be part of resolving it. And happy to review your PR.
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2752412513)
> I'm getting the same issue and it works when
It's still not working for me in a few cases, no matter how many caches I delete or how much I go back in time.
And the failures don't seem to make any sense to me:
```bash
rm -rfd build_fuzz \
&& cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld" \
&& cmake --build build_fuzz \
&& FUZZ=psbt_base64
...
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2752412513)
> I'm getting the same issue and it works when
It's still not working for me in a few cases, no matter how many caches I delete or how much I go back in time.
And the failures don't seem to make any sense to me:
```bash
rm -rfd build_fuzz \
&& cmake --preset=libfuzzer \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_EXE_LINKER_FLAGS="-fuse-ld=lld" \
&& cmake --build build_fuzz \
&& FUZZ=psbt_base64
...
📝 l0rinc reopened a pull request: "doc: use `libfuzzer-nosan` for macOS"
(https://github.com/bitcoin/bitcoin/pull/32084)
After a recent `brew upgrade` (or maybe after the other recent fuzzing changes) I couldn't run the fuzzer anymore, I was getting:
> Linker did not accept requested flags, you are missing required libraries
Changing the preset to avoid the other sanitizers seems to solve the issue.
Since `libfuzzer-nosan` builds to a different folder, I've added the full build steps after configuration.
I've also deleted the `brew install llvm` duplication.
(https://github.com/bitcoin/bitcoin/pull/32084)
After a recent `brew upgrade` (or maybe after the other recent fuzzing changes) I couldn't run the fuzzer anymore, I was getting:
> Linker did not accept requested flags, you are missing required libraries
Changing the preset to avoid the other sanitizers seems to solve the issue.
Since `libfuzzer-nosan` builds to a different folder, I've added the full build steps after configuration.
I've also deleted the `brew install llvm` duplication.
💬 l0rinc commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2752419665)
I'll reopen since others seem to have a similar problem as well - once we figure out what's causing it exactly, we can go back to `--preset=libfuzzer` on mac as well.
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2752419665)
I'll reopen since others seem to have a similar problem as well - once we figure out what's causing it exactly, we can go back to `--preset=libfuzzer` on mac as well.
📝 l0rinc opened a pull request: "fuzz: extract unsequenced operations with side-effects"
(https://github.com/bitcoin/bitcoin/pull/32141)
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced an unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
Tried:
```
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
> g
...
(https://github.com/bitcoin/bitcoin/pull/32141)
https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827 introduced an unsequenced operations with side-effects - which is undefined behavior, i.e. the right hand side can be evaluated before the left hand side, which happens to mutate it.
Tried:
```
clang++ --analyze -std=c++20 -I./src -I./src/test -I./src/test/fuzz src/test/fuzz/base_encode_decode.cpp src/psbt.cpp
```
but it didn't warn about UB.
Grepped for similar ones, but could find any other one in the codebase:
> g
...
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2752457523)
Updated the approach based on the results from the simulations @naumenkogs and I have been running.
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2752457523)
Updated the approach based on the results from the simulations @naumenkogs and I have been running.
🤔 hodlinator reviewed a pull request: "descriptors: Multipath/PR 22838 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2715093563)
Thanks for reviewing! Pushed improvements based on feedback.
(https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2715093563)
Thanks for reviewing! Pushed improvements based on feedback.
💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012887755)
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011731171:
Good catch!
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2012887755)
Thread https://github.com/bitcoin/bitcoin/pull/32134/files#r2011731171:
Good catch!