💬 achow101 commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2356702196)
In 5e9737ff493a96566f4fc11b1733b4b5a5393f6b "wallet: warn against accidental unsafe older() import"

This warning is basically incomprehensible. It would be better to actually write out what it means, i.e. "time based relative locktime > 65535 is unsafe"

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2356706408)
```suggestion
shell: pwsh -Command "$PSVersionTable; $PSNativeCommandUseErrorActionPreference = $true; $ErrorActionPreference = 'Stop'; & '{0}'"
```

🤔 davidgumberg reviewed a pull request: "ci: remove 3rd party js from windows dll gha job"
(https://github.com/bitcoin/bitcoin/pull/32513#pullrequestreview-3236170764)
Nice! Looks good, there are still two places where `shell: pwsh` is used without your ` -Command "$PSVersionTable; $PSNativeCommandUseErrorActionPreference = $true; $ErrorActionPreference = 'Stop'; & '{0}'"` suffix:


https://github.com/bitcoin/bitcoin/blob/c44fb4facfd3a5db8aa1bd2c7f5506a1477b0d2b/.github/workflows/ci.yml#L225

and

Not a big deal for this since it's only using powershell cmdlets now, but for the sake of consistency
https://github.com/bitcoin/bitcoin/blob/c44fb4facfd3a5d
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads 10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3304494917)
> Looks like the CI started failing, due to too many threads being launched in the functional tests with that parallelism? As the threads may open files, this could be hitting the max open files limit? Or maybe it is a different limit hit?

Thanks, I added `-par=1` to all nodes spawned in `features_proxy.py` in 6980852416040bdddf111df3cea3ec50639f010a. That test spawns lots of nodes and block validation is not relevant to it.

> What's the status here?

Rebased to fix silent conflicts and
...

💬 glozow commented on pull request "[28.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/33415#issuecomment-3304507492)
> Should we also backport #33106 if we're going to be doing a 28.3 anyways?

Here is a branch to do this: https://github.com/glozow/bitcoin/tree/2025-09-28.3-backport

Similar to #33226, needed to do some test massaging. I also needed #30948 and #30748 for the test utils/refactors. Feel free to pull; I updated the final changes as well.

💬 hodlinator commented on pull request "build: Remove bitness suffix from Windows installer":
(https://github.com/bitcoin/bitcoin/pull/32132#issuecomment-3304511251)
Maybe we could amend the .NSI-script to unregister versions with the old name. I plan to take a stab at this tomorrow unless something unexpected comes up or someone else wants to pick it up.

💬 achow101 commented on pull request "Fix #25980: Validate transactions in combinerawtransaction":
(https://github.com/bitcoin/bitcoin/pull/33361#issuecomment-3304515191)
#31298 also fixes this issue.

CI is failing because you've added a test that uses wallet behavior to a non-wallet test.

💬 achow101 commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3304517512)
This change breaks the test.

💬 achow101 commented on pull request "rpc: Add validation for invalid taproot signatures in analyzepsbt":
(https://github.com/bitcoin/bitcoin/pull/33360#discussion_r2356755028)
This PSBT isn't serialized correctly.

💬 achow101 commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3304532974)
Please stop using LLMs to generate PRs.

✅ achow101 closed a pull request: "doc: Add documentation explaining different build types"
(https://github.com/bitcoin/bitcoin/pull/33355)

💬 l0rinc commented on pull request "index: remove unnecessary locator cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3304577242)
ACK facd01e6ffbbd019312f370a3807de0b95bbd745

Note that we don't usually add the `Signed off by` to the commit message, it just seems like noise to me. You can still sign your commits if you want, but I'm no sure what this line adds that isn't already obvious.

Also, not sure why `refactor: remove redundant locator cleanup in BaseIndex::Init()` is repeated (slightly differently) in the PR description.

💬 achow101 commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2356816234)
In 65a10fc3c52ea09a4794345bcf607dff908c783a "p2p: add assertion for BlockTransactionsRequest indexes"

Can checking this be done in deserialization directly, rather than iterating all of the indexes again? Since this is part of compact block relay, we want to avoid doing as much unnecessary work as possible.

💬 achow101 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304776460)
> because it used legacy wallet operations

That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.

Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to chang
...

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356963792)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"

This shouldn't be deleted.

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356967273)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"

nit: space between `s` and `:`
```suggestion
for (std::string_view s : strParams) {
```

💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356965832)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"

Extraneous newline

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304806063)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304812339)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...

💬 Raimo33 commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3304898737)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5