💬 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

💬 mzumsande commented on pull request "test: add missing sync to feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/31016#issuecomment-2387293853)
> Should also fix #30640?

Yes! I didn't analyze that in detail but I think that's very likely. I'll add it to the OP.

👍 tdb3 approved a pull request: "test: add missing sync to feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31016#pullrequestreview-2341658200)
code review ACK a1576edab356053c4c736691e4950b58e9a14f76

👍 tdb3 approved a pull request: "doc: add testnet4 section header for config file"
(https://github.com/bitcoin/bitcoin/pull/31007#pullrequestreview-2341683240)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5

Good find. Thanks.

💬 RandyMcMillan commented on pull request "doc/build-osx.md:brew relinking note":
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1783713469)
This PR isn't in response to and issue or comment. I just noticed it may be useful to have a note about relinking.