💬 sdaftuar commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1499507266)
> The correctness of the result of `BuildDiagramFromUnsortedChunks` very much depends on the assumption that the chunks can be reordered w.r.t. one another, which seems to be the case in the current PR, but will very much not be the case going forward in a general cluster mempool world.

No objection to your suggested change here, but I don't follow this comment -- in a general cluster mempool world, unless we were to bound chunk sizes to something smaller than a cluster size (resulting in the
...

⚠️ RHavar opened an issue: "rpc method removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/issues/29466)
### Please describe the feature you'd like to see added.

Currently removeprunedfunds only takes a single txid, however the underlying `CWallet::RemoveTxs` takes an array. I am trying to clean up some wallets, and each individual removeprunedfunds call is very slow. So it'd be a lot better to just be able to pass in all the transactions at once

### Is your feature related to a problem, if so please describe it.

_No response_

### Describe the solution you'd like

_No response_

### Describe a
...

👍 hebasto approved a pull request: "lint: Check for missing bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29408#pullrequestreview-1896313003)
ACK fa58ae74ea67485495dbc2d003adfbca68d6869b, tested on Ubuntu 23.10 -- it catches bugs properly. I didn't review rust code changes.

💬 sr-gi commented on pull request "Choose earliest-activatable as tie breaker between equal-work chains":
(https://github.com/bitcoin/bitcoin/pull/29284#issuecomment-1959818137)
> I don't think that would happen. Since #6010 `nSequenceId` is only set if the block and all of its predecessors have transactions, see [here](https://github.com/bitcoin/bitcoin/blob/2ac41ef15fa523aa381d2b866aaa233b874c42fe/src/validation.cpp#L3627-L3637). Since the attacker never sent us the transactions of block B, C wouldn't get a `nSequenceId` even though we received the full block. C would only receive an `nSequenceId` once the full block B is received, which would then be larger than the
...

🤔 maflcko reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1895813376)
ACK 8afcd99435b693f69b8c3a918b0573ef12559b22 🤗

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 8afcd99435b693f69b8c3a918b
...

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499216412)
594336ae8aad026dacb5d536df63bd374e3a89c7: there there

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499230459)
nit in https://github.com/bitcoin/bitcoin/commit/594336ae8aad026dacb5d536df63bd374e3a89c7: Would be good to avoid the use of `=` in C++, when possible, as it narrowing and possibly confusing when used in contexts with `==`.

```cpp
const bool is_active{c == &ActiveChainstate()};

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499266054)
Just to clarify, the `is_active` check is just a belt-and-suspender, since `pindex == snap_base` implies `is_active`?

An alternative may be to split the assumption out into a separate assert.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1499519079)
Why would the *background* chainstate have the *snapshot* base? The comment seems wrong and the second branch of the if condition is never taken, no?

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-1959849246)
> If this is correct and can be reproduced by others, I think that something should be done about it. If this PR would increase performance for a rare and transient case (right after the users switched their onlynet parameters) but decrease performance for each Select() call in the typical scenario (addrman has mostly addrs from reachable networks), the overall impact on performance of this PR would be clearly negative.
One possibility might be to track the percentage of non-reachable addrs whi
...

💬 hebasto commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1959901619)
Concept ACK on introducing feerate diagram checks for a subset of RBF transactions.

💬 sr-gi commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499599724)
@dergoegge have you considered this?

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1959915076)
> may be an issue with some tests when `testdatadir` is provided a relative path

I can't reproduce this problem. I tried both running the one test alone and running the entire test suite (not specifying `--run_test=`).

💬 glozow commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499592647)
nit c84db8d60e87eabc1e4f88ef05bf06fbf4db6f02

```suggestion
const bool has_witness_commitment{commitpos != NO_WITNESS_COMMITMENT};
```

💬 glozow commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499598937)
cb2437307e6861e0957899345b6e9b93716c383a
Maybe add comment explaining what the tristate return result means? e.g. "bool indicating the witness commitment is correct (state is populated if any error occurs), or std::nullopt if there is no witness commitment to be checked"

💬 glozow commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499595618)
nit c62eea853e5c5f390d58c5ad9c0ac9a688465abc
```suggestion
const auto valid_opt{CheckWitnessCommitment(block, state)};
```

💬 glozow commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499609287)
e2d1eb2e2001c31b43154df47ab5be215a26774f

this is the same thing as
```suggestion
tx = self.wallet.create_self_transfer()["tx"]
```

💬 glozow commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#discussion_r1499613815)
in e2d1eb2e2001c31b43154df47ab5be215a26774f
does this need a `with p2p_lock`?

✅ maflcko closed a pull request: "test: Use test framework utils in functional tests"
(https://github.com/bitcoin/bitcoin/pull/28528)

💬 maflcko commented on pull request "test: Use test framework utils in functional tests":
(https://github.com/bitcoin/bitcoin/pull/28528#issuecomment-1959945805)
Closing for now due to inactivity. I think it is pretty clear that a change like this won't be going in in one go.

If someone wants to try this again, I'd recommend to try a smaller portion and see if reviewers like it. If not, then maybe it isn't worth changing?

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1959949768)
>This contradicts the PR summary:

Thanks for the review! You're right, good catch. I updated the PR description.

The behavior is correct, we don't want to remove the `.lock` file at the start of the run, because we've just _acquired_ the lock, and want to keep it while we delete the test's data directory that might be left over from a previous run. That's why there's a `datadir` at the same level as `.lock` (so we can delete `datadir` without also deleting `.lock`). This behavior was sugge
...