💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108846074)
👍 for comparing size instead of empty - the failure message would likely be more meaningful

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108851720)
I think we could do these in the previous refactor commit instead - to separate low risk changes from ones where we have to may more attention, while keeping the first one about move-only changes + minor ones. Or do the first commit as strictly move-only and add a second one with these tiny refactors (I'd like the latter more).

💬 l0rinc commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2108844548)
Could we use `const std::span<const CBlockHeader>` in these helpers as well? And maybe split these long lines.

👍 l0rinc approved a pull request: "test: Add missing ipc subtree to lint"
(https://github.com/bitcoin/bitcoin/pull/32623#pullrequestreview-2870746798)
Concept ACK, left a consistency nit only

💬 l0rinc commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2108973482)
visiting the links will automatically remove the `.git` suffixes (which was already missing from `libmultiprocess`):
```suggestion
* for `src/crc32c`: https://github.com/bitcoin-core/crc32c-subtree (branch `bitcoin-fork`)
* for `src/crypto/ctaes`: https://github.com/bitcoin-core/ctaes (branch `master`)
* for `src/ipc/libmultiprocess`: https://github.com/bitcoin-core/libmultiprocess (branch `master`)
* for `src/leveldb`: https://github.com/bitcoin-core/leveldb-subtree (branch `bitcoin-fork`)
...

💬 maflcko commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2912220775)
An alternative would be to just not use cmake 4.x and stick with cmake 3.x for now?

🤔 TheCharlatan reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2870684474)
Concept ACK

💬 TheCharlatan commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2108930724)
Just a comment, I think it is kind of fine to do the reorgs through invalidateblock instead of connecting and disconnecting nodes from each other, but the disconnectpool behaviour here is kind of on the line where I would tend to prefer exercising the actual code through `ActiveBestChainStep`, since the two methods do not share the same object. I think to test the mempool in general it is fine to do, both methods at least use the same object at runtime.

⚠️ azazar opened an issue: "Assertion failed: TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state) (wallet/wallet.cpp: AddToWallet: 1094)"
(https://github.com/bitcoin/bitcoin/issues/32625)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Bitcoin Core v27.0.0 crashes with an assertion failure in `wallet.cpp` at line 1094, related to `TxStateSerializedIndex(wtx.m_state) == TxStateSerializedIndex(state)`.

* **Bitcoin Core Version**: v27.0.0 (release build)
* **Platform**: Linux (Testnet)
* **Configuration**:

* `consolidatefeerate=0.00000000`
* `dbcache=450`
* `fallbackfee=0.00005`
* `limitancestorcount=1000`
* `ma
...

🤔 maflcko reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2870355565)
lgtm ACK 2b202e9db56487e658fc41089178f31ef4259a0d 🛤

<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: lgtm ACK 2b202e9db56487e6
...

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2109018971)
nit in 1aeb1838cabf63507a2885e50499d94588f0fd83: Can drop the call here and use the `mempool` from the next line?

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2108700980)
```suggestion
# At the end the old fork is revalidated to cause reorg, and all of the created
```

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2109024282)
nit in 2b202e9db56487e658fc41089178f31ef4259a0d: Could force named args, while touching this?

```py
... size, *, n_tx_to...

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2109020817)
nit in 05abcc86998f6649b48ed3a2eaf2a024ba42af5c: Remove the comment, or add "coins to test", or "coins for test"?

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2109025020)
(nit in https://github.com/bitcoin/bitcoin/commit/2b202e9db56487e658fc41089178f31ef4259a0d:)

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2109025763)
nit in https://github.com/bitcoin/bitcoin/commit/2b202e9db56487e658fc41089178f31ef4259a0d: Any reason to remove this here?

💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2109038105)
nit in https://github.com/bitcoin/bitcoin/commit/2b202e9db56487e658fc41089178f31ef4259a0d: could clarify the exact error string?

```
too-long-mempool-chain, too many unconfirmed ancestors [limit: 100]

💬 maflcko commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#discussion_r2109053322)
thx, I'll leave as-is for now to make it possible to review via `--color-moved=dimmed-zebra`. Seems better to be done as a repo-wide follow-up anyway, because this affects all docs.

💬 l0rinc commented on pull request "test: Add missing ipc subtree to lint":
(https://github.com/bitcoin/bitcoin/pull/32623#issuecomment-2912331467)
ACK fa2bf6bdb7e5f8940fe4bce1aaab29bf2e063b49

💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2912362918)
https://cirrus-ci.com/task/4713767570767872?logs=ci#L3220

💬 maflcko commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2109086986)
why is this needed? Isn't the chain already `MAIN`?