💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389245944)
This seems a bit out of scope of this PR, but I think you are right, I don't see the need for `::cs_main` here either. Maybe open a separate PR for this?

👍 hebasto approved a pull request: "Bugfix: configure: Correct check for fuzz binary needing a main function"
(https://github.com/bitcoin/bitcoin/pull/28564#pullrequestreview-1724552391)
ACK 8f6ca584cd5c0b4882ce6f3c158b6f23aed8f6dc.

The `./configure` script logs are as follow:
- in the master branch:
```
checking whether main function is needed for fuzz binary... checking whether the linker accepts ... no
yes
```
or
```
checking whether main function is needed for fuzz binary... checking whether the linker accepts ... yes
no
```
- in this PR branch:
```
checking whether main function is needed for fuzz binary... yes
```
or
```
checking whether main function i
...

📝 TheCharlatan opened a pull request: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843)
Instead, pass the mempool pointer as an argument to CreateNewBlock and pass that on to addPackageTxs. Before, addPackageTxs had access to both the m_mempool member and the passed in mempool, which could be confusing. This was noticed in this PR review: https://github.com/bitcoin/bitcoin/pull/25223#discussion_r898164322.

👍 maflcko approved a pull request: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843#pullrequestreview-1724561168)
lgtm

💬 maflcko commented on pull request "Bugfix: configure: Correct check for fuzz binary needing a main function":
(https://github.com/bitcoin/bitcoin/pull/28564#issuecomment-1805531566)
So this is only to fix a test-only debug-only output? Not sure. I guess it can be merged, if cmake does not make it into 27.x. Otherwise it seems odd to change this, when the code will be removed either way before any user will even run or see it.

👋 TheCharlatan's pull request is ready for review: "[refactor] Remove BlockAssembler m_mempool member"
(https://github.com/bitcoin/bitcoin/pull/28843)

💬 fjahr commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805593065)
> Yes, this patch is reducing the value.

Maybe as a side-effect but it's rolling back the randomization completely. What I mean is reducing the max value of `seek`, i.e. 15000 to something lower that is safe.

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389240397)
nit in c14e589817035e76e2707fbcd5cb99689d42de77: If you call `Assert` before an assignment, it could make sense to turn it into a reference by calling `*Assert()`. This makes later code smaller and easier to read.

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389248596)
5f72a579de74a6d911ca8529fe8754995025d61a: Same?

Also, could squash into the previous `GetEntry` commit?

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389292338)
91df77fde5adc7e2e785aa0d60b82a8515c9b538: Is the lock needed?

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389241558)
same

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389293490)
nit in the first commit:

```
clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/txmempool.cpp
txmempool.cpp:848:13: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
848 | ret.push_back(*it);
| ^~~~~~~~~~
| emplace_back(

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389287348)
6635cffb1e17ad5fdfdabe68cbdcaf1c68d8070d:

Not sure about turning (theoretical) UB into an exception throw. Might be better to turn it into an assert fail by using `Assert(GetEntry())->`?

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389274834)
nit in 1598b906a57bcd5fc50c1945d8420f8ace1d9c34: Could use `get` or `GetEntry`, when only access to the tx pointer is needed, and not full access to the internal iterator?

💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805595495)
> Maybe as a side-effect but it's rolling back the randomization completely. What I mean is reducing the max value of `seek`, i.e. 15000 to something lower that is safe.

Why? Did you see my previous comment?

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805596963)
ACK 91df77fde5adc7e2e785aa0d60b82a8515c9b538 🕐

<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: ACK 91df77fde5adc7e2e785aa0d
...

💬 dergoegge commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1805604074)
Coverage after 1000 CPU hours of fuzzing: https://dergoegge.github.io/bitcoin-coverage/pr26812/fuzz.coverage/index.html

Doesn't reach much in net_processing, so it's probably not doing much in terms of e2e testing.

💬 dergoegge commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1389308579)
We probably want to do what marco did in #28815 here, i.e. break out of the loop on "bad" data

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389337349)
No, but does the other one in the `while` loop make sense?

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1805671306)
Will address the outstanding comments shortly.