💬 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)

💬 yancyribbens commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894540348)
> The presence of BnB ensures that these expected input set will be found. The presence of BnB does neither preclude another algorithm from finding the same solution, nor cause the BnB solution to be necessarily picked among two equivalent solutions produced by two different algorithms. Hence it does not make sense to require that the solution was produced by BnB, only that the best solution will be found.

I noted earlier that my objection is that this test should go elsewhere then if it's no
...