π¬ maflcko commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2682393121)
There is also a risk of adding even more regressions due to `CCACHE_NOHASHDIR`. I don't know if it is related, but currently one can not use `callgrind_annotate`, even with `--include=<src_dir>`. It is likely unrelated, but I wonder if the patch here makes it worse.
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2682393121)
There is also a risk of adding even more regressions due to `CCACHE_NOHASHDIR`. I don't know if it is related, but currently one can not use `callgrind_annotate`, even with `--include=<src_dir>`. It is likely unrelated, but I wonder if the patch here makes it worse.
π costcould opened a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31952)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31952)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
π¬ maflcko commented on pull request "chore: remove redundant word":
(https://github.com/bitcoin/bitcoin/pull/31952#issuecomment-2682483302)
lgtm ACK d9ba427f9d09dcf39cfdc7f871f7b093b876a0a3
(https://github.com/bitcoin/bitcoin/pull/31952#issuecomment-2682483302)
lgtm ACK d9ba427f9d09dcf39cfdc7f871f7b093b876a0a3
π maflcko opened a pull request: "rpc: Allow fullrbf fee bump in (psbt)bumpfee"
(https://github.com/bitcoin/bitcoin/pull/31953)
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is the first of a series of rbf clean-ups.
(https://github.com/bitcoin/bitcoin/pull/31953)
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is the first of a series of rbf clean-ups.
π fanquake merged a pull request: "chore: remove redundant word"
(https://github.com/bitcoin/bitcoin/pull/31952)
(https://github.com/bitcoin/bitcoin/pull/31952)
π¬ Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1970180102)
What I meant here was that `clang/llvm` comes installed by default in the machines (although the entire llvm suite might not be there). `lcov` generally has to be downloaded before using in almost every machine.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1970180102)
What I meant here was that `clang/llvm` comes installed by default in the machines (although the entire llvm suite might not be there). `lcov` generally has to be downloaded before using in almost every machine.
π¬ luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1970203305)
Just below here... the `#define` for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1970203305)
Just below here... the `#define` for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.
π¬ Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2682638796)
Addressed the comments
1. Change the commands so we dont have to change shell from the root of the repo.
2. Added a comment for MacOs users to specify the command changes needed to use different llvm version if faced with issues.
(https://github.com/bitcoin/bitcoin/pull/31933#issuecomment-2682638796)
Addressed the comments
1. Change the commands so we dont have to change shell from the root of the repo.
2. Added a comment for MacOs users to specify the command changes needed to use different llvm version if faced with issues.
π¬ maflcko commented on pull request "test: Move `script_assets_tests` into its own suite":
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-2682715106)
Seems fine. No strong opinion.
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-2682715106)
Seems fine. No strong opinion.
π¬ secp512k2 commented on issue "ci: failure in interface_usdt_utxocache.py":
(https://github.com/bitcoin/bitcoin/issues/31951#issuecomment-2682974300)
The test failure in `interface_usdt_utxocache.py` occurs due to a **byte-order mismatch** when comparing the expected UTXO `txid` with the actual USDT probe event.
**Root Cause**:
The test reverses the byte order of `event.txid` before converting it to a hex string (`bytes(event.txid[::-1]).hex()`), but the generated transactionβs `txid` (from RPC) is already in the correct display format. This unnecessary reversal causes the assertion failure.
**Proposed Fix**:
Remove the byte reversal
...
(https://github.com/bitcoin/bitcoin/issues/31951#issuecomment-2682974300)
The test failure in `interface_usdt_utxocache.py` occurs due to a **byte-order mismatch** when comparing the expected UTXO `txid` with the actual USDT probe event.
**Root Cause**:
The test reverses the byte order of `event.txid` before converting it to a hex string (`bytes(event.txid[::-1]).hex()`), but the generated transactionβs `txid` (from RPC) is already in the correct display format. This unnecessary reversal causes the assertion failure.
**Proposed Fix**:
Remove the byte reversal
...
π¬ pinheadmz commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2682988681)
I'm still tweaking the .lldbinit file actually -- sometimes I'll be debugging with lldb and end up in a new directory I hadn't linked yet like `/fuzz` or something and it'll break again. So I suspect there is a simple catch all solution I just haven't figured it out yet
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2682988681)
I'm still tweaking the .lldbinit file actually -- sometimes I'll be debugging with lldb and end up in a new directory I hadn't linked yet like `/fuzz` or something and it'll break again. So I suspect there is a simple catch all solution I just haven't figured it out yet
π fanquake merged a pull request: "cmake: Add optional sources to `minisketch` library directly"
(https://github.com/bitcoin/bitcoin/pull/31880)
(https://github.com/bitcoin/bitcoin/pull/31880)
π€ glozow reviewed a pull request: "test: add coverage for immediate orphanage eviction case"
(https://github.com/bitcoin/bitcoin/pull/31628#pullrequestreview-2642192070)
I don't think it hurts to have coverage for this. Gripes would be (1) this isn't necessarily the behavior we want, just the behavior we have (2) it uses `-maxorphantxs=0` and imo we probably don't care to keep this config option / a limit on count long term? So ACK assuming we are relaxed about removing it later.
(https://github.com/bitcoin/bitcoin/pull/31628#pullrequestreview-2642192070)
I don't think it hurts to have coverage for this. Gripes would be (1) this isn't necessarily the behavior we want, just the behavior we have (2) it uses `-maxorphantxs=0` and imo we probably don't care to keep this config option / a limit on count long term? So ACK assuming we are relaxed about removing it later.
π¬ glozow commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#discussion_r1970398055)
Would probably move this to the very end, given what we know about `getorphantxs` taking a while to update
(https://github.com/bitcoin/bitcoin/pull/31628#discussion_r1970398055)
Would probably move this to the very end, given what we know about `getorphantxs` taking a while to update
π¬ Andrewsr84 commented on issue "Bitcoin is returning higher fees for 36 block window than 2 block window (on testnet)":
(https://github.com/bitcoin/bitcoin/issues/11800#issuecomment-2683110694)
Did all issue get fixed
(https://github.com/bitcoin/bitcoin/issues/11800#issuecomment-2683110694)
Did all issue get fixed
π¬ janb84 commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2683121579)
Tested ACK [ba82240](https://github.com/bitcoin/bitcoin/pull/31870/commits/ba82240553ddd534287845e10bc76b46b45329fe)
The change to fuzz each algorithm independently seems an okay idea. The implementation/separation in the code also makes it easier to read, review, and reason about.
- Built and ran fuzzers, minimum of 15 minutes each.
- Reviewed code changes line-by-line
- Tried to compile CodeCoverage of new Fuzz targets but I failed at that (not a code issue)
I also wrote a [review
...
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2683121579)
Tested ACK [ba82240](https://github.com/bitcoin/bitcoin/pull/31870/commits/ba82240553ddd534287845e10bc76b46b45329fe)
The change to fuzz each algorithm independently seems an okay idea. The implementation/separation in the code also makes it easier to read, review, and reason about.
- Built and ran fuzzers, minimum of 15 minutes each.
- Reviewed code changes line-by-line
- Tried to compile CodeCoverage of new Fuzz targets but I failed at that (not a code issue)
I also wrote a [review
...
π¬ m3dwards commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2683127943)
> Trying to access https://corecheck.dev/bitcoin/bitcoin/pulls/31923 gives me
Should be sorted
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2683127943)
> Trying to access https://corecheck.dev/bitcoin/bitcoin/pulls/31923 gives me
Should be sorted
π¬ darosior commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1970460482)
Done.
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1970460482)
Done.
π¬ ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2683164191)
Rebased da25311e3a88941bf99a732be1aa3568a65f2273 -> c2c5a0f492ae2ce54afdd031e8a2f2689a8c4942 ([`pr/subtree.19`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.19) -> [`pr/subtree.20`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.19-rebase..pr/subtree.20)) with no changes after #31945 merge
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2683164191)
Rebased da25311e3a88941bf99a732be1aa3568a65f2273 -> c2c5a0f492ae2ce54afdd031e8a2f2689a8c4942 ([`pr/subtree.19`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.19) -> [`pr/subtree.20`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.20), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.19-rebase..pr/subtree.20)) with no changes after #31945 merge
π¬ Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2683198821)
Rebased on the latest #30975, CI is not amused.
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2683198821)
Rebased on the latest #30975, CI is not amused.