💬 m3dwards commented on issue "IBD performance regression in 27.0rc1 on Windows":
(https://github.com/bitcoin/bitcoin/issues/29785#issuecomment-2061714375)
I did get a bit of variation but hard to know what's important. `BlockAssemblerAddPackageTxns` was about 10-15% slower on `27.0rc1` vs `26.0`. The bench also throws a filesystem error on 27.0rc1 but completes on 26.0.

[Full bench results](`BlockAssemblerAddPackageTxns`)

💬 stratospher commented on issue "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError":
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2061766412)
seen in #29898 too - https://cirrus-ci.com/task/5457627730149376?logs=ci#L3718

🚀 ryanofsky merged a pull request: "security: restrict abis in bitcoind.service"
(https://github.com/bitcoin/bitcoin/pull/28340)

🤔 murchandamus reviewed a pull request: "fuzz: wallet, add target for Spend"
(https://github.com/bitcoin/bitcoin/pull/28236#pullrequestreview-2006620250)
Hey,

Just did a quick pass. What is the status of this PR?

💬 murchandamus commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1569162422)
This comment is inconsistent:

```suggestion
// Add 50 spendable UTXO, 50 BTC each, to the wallet (total balance 2500 BTC)
```

A variable set of available coins might be desirable. Smaller amounts could for example trigger behavior regarding negative effective value or having insufficient funds to create a transaction.

💬 murchandamus commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1569167950)
If you removed the Singleton, did you perhaps not push the latest state of this PR?

💬 murchandamus commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1569173834)
Agree wtih @maflcko, since the seeds are created per target, it would be better to have a separate target for `CoinsResult`.

💬 maflcko commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2061781675)
Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding regression fuzz tests, so here are some ideas:

* #23444 tried to add a regression fuzz test, but it was picked up as part of https://github.com/bitcoin/bitcoin/pull/28933, and I couldn't reproduce the regression crash by re-introducing it.
* https://github.com/bitcoin/bitcoin/pull/27
...

💬 petertodd commented on pull request "tests: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2061785140)
On Wed, Apr 17, 2024 at 04:32:14AM -0700, Chris Stewart wrote:
> @dgpv @petertodd what do you think is the path forward for this?

Seems to me that one reasonable path forward is to just do nothing and not
merge these changes.

🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2006530259)
I think we had some docs written for doc/policy/packages.md and doc/policy/mempool_replacements.md?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1569130553)
This seems like a merge conflict with #28970, maybe add the fix here instead?

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1569134261)
34c513554c7be97d91446126bb3ef0de2884fc19

This seems slightly inaccurate, as it wouldn't currently be in a package if `ReplacementChecks` is happening?

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-2061802631)
> Tested with HWI 2.4.0
>
> When a Ledger Nano X is in screen saver mode, I now get the following error:
>
> ```
> '...hwi' error: Could not open client or get fingerprint information: ('0x5515', 'Error in <DefaultInsType.GET_VERSION: 1> command', '')
> ```
>
> That's what HWI returns, so can't make it much better...
>
> When the Bitcoin app is not opened on the device the error is more clear:
>
> ```
> '...hwi' error: Could not open client or get fingerprint information: Ledge
...

💬 maflcko commented on pull request "Bugfix: RPC: Check for blank rpcauth on a per-param basis":
(https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-2061840567)
Are you still working on this?

💬 maflcko commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#issuecomment-2061844761)
Are you still working on this?

💬 theuni commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2061848543)
@hebasto Is there a venue for reporting this to MSVC? They recently [patted themselves on the back for detecting similar patterns](https://devblogs.microsoft.com/cppblog/a-tour-of-4-msvc-backend-improvements/). It's a shame MSVC can't detect something that (in 2024) seems so obvious.

👍 ryanofsky approved a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2006662114)
Code review ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26, but I think `reindex` /`m_reindex` naming is a little confusing so I left some suggestions below to clean it up.

I also really like stickies suggestion from https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868 to delete the `ChainstateLoadOptions::reindex` variable and think it would be great to add it as a second commit. If you do add it, I also think it would also be good idea to add a third commit renaming `Cha
...

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1569186270)
In commit "kernel: De-globalize fReindex" (d9bcbbf2293ef427b37eefca30f074be5eeeca26)

Note: removing this line does not change behavior, because `GetBoolArg("-reindex")` is now called a few lines below on line 1501 in `ApplyArgsManOptions(args, blockman_opts)`

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1569209547)
In commit "kernel: De-globalize fReindex" (d9bcbbf2293ef427b37eefca30f074be5eeeca26)

I think `std::atomic<bool>& reindex` should be replaced by `bool reindexing` here. Calling it `reindexing` would make the name consistent with the block manager `m_reindexing` member (suggested above) which this is referencing. Avoiding std::atomic would make the function more deterministic, and avoiding the non-const reference would make it clear the value will not be modified.

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1569200553)
In commit "kernel: De-globalize fReindex" (d9bcbbf2293ef427b37eefca30f074be5eeeca26)

I don't think `m_reindex` is good name for this variable because it is easy to get confused with the `reindex` option which reflects whether reindexing was requested, not whether it is currently happening. Would suggest calling this `m_reindexing` and adding a comment that this reflects whether reindexing is currently happening, and is set to true when reindexing is requested, and false when reindexing comple
...