Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 maflcko commented on issue "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust":
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2061604188)
I guess it could make more sense if someone tried to reproduce this in `rr`.
💬 theStack commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#issuecomment-2061659300)
Concept ACK, thanks for fixing!
⚠️ brunoerg opened an issue: "Wallet fuzzing tracking issue"
(https://github.com/bitcoin/bitcoin/issues/29901)
The wallet has poor fuzz coverage. Hopefully, some work is being done to improve it. The goal of this issue is to actively track current work and work that needs to be done to improve fuzz coverage for the wallet.

## Open PRs / Ready to review

- [ ] https://github.com/bitcoin/bitcoin/pull/29694
- [ ] https://github.com/bitcoin/bitcoin/pull/28236
- [ ] https://github.com/bitcoin/bitcoin/pull/28074

## Current wallet targets

We currently have 6 specific targets for wallet, which cover
...
💬 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
...