💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805828533)
> If someone is working on a follow-up introducing the random perturbation, feel free to ping me as reviewer.

Same here, happy to review

💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1389467446)
Seems fine to report an issue about this, with exact steps to reproduce.

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805841682)
re-ACK 3b73acb3ec678f5099aa0d77178a0b0d50670c2b 🏎

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 3b73acb3ec678f5099aa
...

📝 fanquake opened a pull request: "depends: remove `PYTHONPATH` from config.site"
(https://github.com/bitcoin/bitcoin/pull/28845)
We no-longer need this, as we no-longer build python packages.

Guix Build (aarch64):
```bash
92c922498b9d7e68742355653cc84317d0f0947e8fdd012488c9875e8b54e03c guix-build-95aab1f02784/output/aarch64-linux-gnu/SHA256SUMS.part
634a6a7a82efcb86eb7e875aa4534bd5646e80ee21e951d9624ca7ff78fc82f5 guix-build-95aab1f02784/output/aarch64-linux-gnu/bitcoin-95aab1f02784-aarch64-linux-gnu-debug.tar.gz
2dd4305e325723cac4b0b8e1803d48f3d7eb6fa2fd49c0a980ff907bb3e5da2b guix-build-95aab1f02784/output/aarch
...

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1389521297)
Ok, lets use an example (from the newly added test):

`vToFetch = [2 ... 129]`
`state->pindexBestKnownBlock->nHeight = 301`
So the peer's height is `301`. We can assume they can provide blocks `[13 ... 301]`.
At the first iteration of the loop `pindex->nHeight` is 2 (`vToFetch[0]`).
The code ends up with `return;` because `301 - 2 > 286` and we never request any blocks from that peer.

Shouldn't we request blocks `[13 ... 301]` instead?

💬 vasild commented on pull request "p2p: make block download logic aware of limited peers threshold":
(https://github.com/bitcoin/bitcoin/pull/28120#discussion_r1389526651)
The test would fail if run standalone without the preceding tests. The failure is on line 78: `self.sync_blocks([miner, pruned_node])`. That is because the test relies on the pruned node being connected to the miner which is indeed the case - some leftover from previous tests. This diff makes it succeed when run standalone:

```diff
--- i/test/functional/p2p_node_network_limited.py
+++ w/test/functional/p2p_node_network_limited.py
@@ -54,25 +54,24 @@ class NodeNetworkLimitedTest(BitcoinTest
...

💬 L0laL33tz commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805920351)
Cool, Im working on a new follow-up excluding the leveldb from randomization @maflcko @theStack

👍 stickies-v approved a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1724824472)
ACK 3b73acb3ec678f5099aa0d77178a0b0d50670c2b

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389421589)
nit: can use `CTxMemPoolEntryRef`

<details>
<summary>git diff on 3b73acb3ec</summary>

```diff
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 85c2195b13..7c905ca4f4 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -176,4 +176,6 @@ public:
mutable Epoch::Marker m_epoch_marker; //!< epoch when last touched, useful for graph algorithms
};

+using CTxMemPoolEntryRef = CTxMemPoolEntry::CTxMemPoolEntryRef;
+
#endif // BITC
...

💬 maflcko commented on pull request "depends: remove `PYTHONPATH` from config.site":
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806001689)
When was this last used?

💬 fanquake commented on pull request "depends: remove `PYTHONPATH` from config.site":
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806003917)
> When was this last used?

When we were building DMGs for macOS.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389594932)
Will push this, seems easy enough to re-ACK.

💬 kashifs commented on pull request "BIP324 integration":
(https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1389597811)
@sipa, am I missing something or should this line read:

`node_1_info = self.nodes[1].getpeerinfo()`

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1806011505)
Updated 3b73acb3ec678f5099aa0d77178a0b0d50670c2b -> 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b ([simplifyMemPoolInteractions_15](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_15) -> [simplifyMemPoolInteractions_16](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_16), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_15..simplifyMemPoolInteractions_16))

* Applied @stickies-v's [patch](https://github.com/bitc
...

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389606616)
If you put this in the global namespace, might as well remove the duplicate from the inner scope?

💬 sipa commented on pull request "BIP324 integration":
(https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1389608289)
This indeed looks like a bug.

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389615076)
Can it be replaced without declaration order issues?

💬 stickies-v commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389641144)
I tried that at first but couldn't figure a more concise way given the `Parents` and `Children` typedefs.

👍 stickies-v approved a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1725182406)
re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b

💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1806089364)
reACK 4dd94ca