💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-1894414662)
Converting to draft until I get around to fixing the coin_selection benchmark.

💬 achow101 commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1894420211)
ACK cdc6ac4126b31426261605a757c52ea2dbfb2a81

💬 ismaelsadeeq 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_r1453480372)
```suggestion
/** Add fee and size of another FeeFrac to this one. */
void inline operator+=(const FeeFrac& other) noexcept
{
fee += other.fee;
size += other.size;
}

/** Subtrack fee and size of another FeeFrac from this one. */
```

💬 ismaelsadeeq 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_r1453482107)
No need for this return here
```suggestion
}
```

💬 ismaelsadeeq 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_r1453501485)
nit: we can just use the empty constructor
```suggestion
FeeFrac empty{};
```

💬 ismaelsadeeq 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_r1453480679)
Reserve here?
```suggestion
diagram.clear();
diagram.reserve(chunks.size() + 1);
```

💬 ismaelsadeeq 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_r1453487633)
Maybe add a docstring on what `BuildDiagramFromUnsortedChunks` does in and out params description that are doxygen compatible?

💬 ismaelsadeeq 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_r1453502118)
why can we have negative size ?

💬 ismaelsadeeq 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_r1453612523)
use static cast?
```suggestion
FeeFrac package{txiter->GetModFeesWithAncestors(), static_cast<int32_t>(txiter->GetSizeWithAncestors())};
```

💬 ismaelsadeeq 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_r1453626393)
I understand what happens here but its a bit confusing to me the short forms `CON`, `CNK` `DIAGRAM`
Will prefer something like the compute in computing the `OLD` chunk

```suggestion
// Step 2: build the new diagram

// Add any parents of direct conflicts that are not conflicted themselves into the NEW chunk
for (auto direct_conflict : direct_conflicts) {
// If a direct conflict has an ancestor that is not in all_conflicts,
// it can be affected by the replac
...

💬 ismaelsadeeq 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_r1453604924)
I think you mean `all_conflicts` here
```suggestion
// old diagram will consist of each element of all_conflicts either at
```

💬 ismaelsadeeq 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_r1453667765)
Should forward the detailed error message also?

🚀 achow101 merged a pull request: "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`"
(https://github.com/bitcoin/bitcoin/pull/28791)

💬 achow101 commented on pull request "[26.x] more backports":
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1894430019)
perhaps add #28791

💬 achow101 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1453968110)
In 83c807a380b41787214806c6440236ea1a9dabd7 "sqlite: introduce HasActiveTxn method"

`HasActiveTxn()` checks for `m_db` already, so no need to do check that here too.

Same for `TxnCommit()` and `TxnAbort()`.

💬 jamesob commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1894442242)
Post-merge ACK https://github.com/bitcoin/bitcoin/commit/cdc6ac4126b31426261605a757c52ea2dbfb2a81

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1894446416)
Reverted this to b316a81704fdd7fc5984ee8c189a937302b70c83 as #29253 deals with the problem of failing `TxnAbort()` and `TxnCommit()`.

💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1894453578)
Probably neither here nor there, but didn't see much of a performance impact from this.


💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1453982729)
upps done

👋 murchandamus's pull request is ready for review: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366)