💬 davidgumberg commented on pull request "Don't zero-after-free `DataStream`: ~25% faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2387137498)
> I tested this PR on an AMD Ryzen 7950x machine with Ubuntu 24.04, with one local network peer and a gigabit internet connection.
>
> ```
> bitcoind -dbcache=30000 -stopatheight=863000 -blocksdir=/magnetic/.bitcoin -addnode=local-network
> ```
>
> Before: 5 hours 10 minutes After: 4 hours and 55 minutes
>
> Time includes 20 minutes to flush chainstate to disk during shutdown.

@Sjors Thanks for testing! I suspect that the biggest improvements will be seen on memory bandwidth constr
...

🤔 ismaelsadeeq reviewed a pull request: "test: switch MiniWallet padding unit from weight to vsize"
(https://github.com/bitcoin/bitcoin/pull/30718#pullrequestreview-2341539943)
post-merge ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d

This PR simplify `_bulk_tx` a lot.

📝 mzumsande opened a pull request: "test: add missing sync to feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31016)
This fixes a race:

- In the `test_estimate_dat_is_flushed_periodically` subtest, node 0 is isolated and creates 10 blocks (no sync).
- In `clear_estimates` the nodes are reconnected (but we don't wait for them to sync!)
- In the `sanity_check_rbf_estimates` subtest, node 1 generates another block and syncs with the other nodes. The sync fails if the generated block is at the same height as the tip of the node.

Fix this by adding a sync to `clear_estimates`.

Fixes #30990

💬 mzumsande commented on issue "ci: Intermittent issue in feature_fee_estimation.py in sanity_check_rbf_estimates (Block sync timed out after 2400s)":
(https://github.com/bitcoin/bitcoin/issues/30990#issuecomment-2387172634)
Looks like a sync was missing, see #31016 for a fix.

💬 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