💬 Richard-23-y commented on issue "Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2387189486)
Hello there

🤔 ismaelsadeeq reviewed a pull request: "test: add missing sync to feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31016#pullrequestreview-2341572804)
Concept ACK

Thanks for solving the issue

Should also fix #30640

🤔 hodlinator reviewed a pull request: "Add a "tx output spender" index"
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-2341436782)
Reviewed 93e1cafc3f8a7f5b36e495d7eb68c19acdab5809

Concept: I am curious about the justification for including this Index in Bitcoin Core vs implementing it as a third party index, especially since it is not required by anything inside of Core itself. It keeps the index in close sync with the chain state and de-duplicates effort among L2-implementations, but adds maintenance burden in Core.

Implementation: Appreciate the fresh SipHash take. Solves the DoS problem and has some nerd-sniping p
...

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783556338)
nit: More consistent with type name and snake_case variable naming convention.
```suggestion
bool TxoSpenderIndex::ReadTransaction(const CDiskTxPos& tx_pos, CTransactionRef& tx) const
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783555986)
(Feel free to resolve https://github.com/bitcoin/bitcoin/pull/24539/files#r1706624715).

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783550973)
Manually adding `__func__` to new log lines is discouraged since `-logsourcelocations` CLI arg was added.
```suggestion
LogWarning("Could not read expected entry");
```
Here and elsewhere in the file.

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783559400)
Consider renaming function to `CreateKey` since underlying LevelDB is key/value store. "Index" also collides with the `TxoSpenderIndex` name itself.

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783548346)
(Feel free to resolve https://github.com/bitcoin/bitcoin/pull/24539/files#r1708031905).

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783554867)
Maybe avoid double-lookup in Release, depending on how unlikely/harmful you feel this is?
```suggestion
if (!m_db->Read(key, positions)) {
Assume(!m_db->Exists(key)); // Cannot read current state; tx spender index may be corrupted
}
```

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1783558667)
Should at least have `m_`-prefix (possibly also distinguishing it as a "salt" instead of a "key", but I understand key is consistent with SipHash terminology).
```suggestion
std::pair<uint64_t, uint64_t> m_siphash_salt;
```

It's unfortunate that `SaltedOutpointHasher` is not directly usable here. Would require serialization support at the very least.

💬 ismaelsadeeq commented on pull request "test: raise an error in `_bulk_tx_` when `target_vsize` is too low":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1783615660)
Done, thanks.

💬 ismaelsadeeq commented on pull request "test: raise an error in `_bulk_tx_` when `target_vsize` is too low":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1783615744)
Done

💬 ismaelsadeeq commented on pull request "test: raise an error in `_bulk_tx_` when `target_vsize` is too low":
(https://github.com/bitcoin/bitcoin/pull/30322#issuecomment-2387228871)

Rebased after #30718

The rationale behind this PR was to improve the `_bulk_tx` method. Some of the commits here are no longer relevant after #30718; however, I believe the ones included in the latest push are still significant.

The PR now consists of four trivial but useful commits, IMO:

- 960fc3a20947a6282ff065e98db63f3ef0bdfc36: Removes the duplicate `assert` statement in `test_tx_padding` since it would be dead code in case of failure.
- 0f1b964c22fee180542fd72de6001ef350fee29b:
...

💬 sdaftuar commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2387271117)
ACK 2b21aebd9754c503bac12d1dbf566b3aa27451e8

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1783656759)
Moved to `mempool_util.py`

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1783657081)
Added check

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1783657306)
Updated

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1783657604)
Updated

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1783657742)
Updated

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2387291364)
Updated to e6853592361341c27103ed74b25470ac1e098d6d to address comments from @hodlinator and @itornaza