💬 fanquake commented on pull request "Remove now-unnecessary poll, fcntl includes from net(base).cpp":
(https://github.com/bitcoin/bitcoin/pull/27530#issuecomment-1540387236)
The scope of this has expanded, and it now conflicts with a number of things, that are much higher priority. You could wait until all the kernel / assumeutxo changes are merged, (and maybe also 27571, to remove the need to update the list), or, revert back to the original non-conflicting change, and that could probably be merged. Otherwise, I think this is likely to just sit for some time, and mostly generate merge-conflict noise in other drahtbot/other PRs.

💬 fanquake commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#discussion_r1188780365)
In 9bcbbc68682c1a92316d6a2dd8142839c58fe247 (fuzz, refactor: hoist net_len_map in addrman fuzzer to constant): Isn't this already a constant? Seems to just be moving the code out of the function where it's used?

💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#issuecomment-1540397377)
Rebased.

🤔 glozow reviewed a pull request: "net processing: avoid serving non-announced txs as a result of a MEMPOOL message"
(https://github.com/bitcoin/bitcoin/pull/27602#pullrequestreview-1418933543)
Concept ACK

The issue on master that I believe this addresses is being able to learn exactly when a transaction enters your mempool. You send `mempool` and get the response, then put out the tx, then `getdata getdata getdata getdata...` until the node tells you the tx instead of `notfound`. Also in general I think it's quite sensible to only serve transactions you announced.

💬 fanquake commented on pull request "refactor: Replace global find_value function with UniValue::find_value method":
(https://github.com/bitcoin/bitcoin/pull/27605#issuecomment-1540432168)
With fa0d180f48d4e5253f58aced32768369f963d1c7 & `gcc (GCC) 13.1.1 20230426 (Red Hat 13.1.1-1)`, fixes all issues from #26926 except for:
```bash
test/interfaces_tests.cpp: In member function ‘void interfaces_tests::findCommonAncestor::test_method()’:
test/interfaces_tests.cpp:101:19: warning: possibly dangling reference to a temporary [-Wdangling-reference]
101 | const CChain& active = WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return Assert(m_node.chainman)->ActiveChain());

...

💬 glozow commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540433286)
> This is true for 2 (two) full nodes that run with pruning off, peerbloomfilters off, maxmempool set to 2048 and maxconnections set to 500, where both have an average between 170 and 225 active peers.

This particular case seems like you have configured bitcoind to do more than your cpu can handle during higher-traffic times. Consider reducing maxconnections for now. And perhaps shrink maxmempool - may I ask why you are using 2048?

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042)
Well, I mean that the `default` case can now be skipped. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures section `enum class` - `default:`

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847896)
(Otherwise, if a new chain type is added, it will silently map to `main`)

💬 hebasto commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1540490305)
Reworked according to @MarcoFalke's [suggestion](https://github.com/bitcoin/bitcoin/pull/27598#issuecomment-1538554964).

Friendly ping @martinus :)

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188851857)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188852707)
What is the point of adding dead code over `assert(false)`? Also, the docstring is now wrong. See also https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1188847042

💬 MarcoFalke commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540528281)
See also https://github.com/bitcoin/bitcoin/pull/14309 , where I added the ChainType enum class.

💬 st3b1t commented on pull request "rpc: append rpcauth.py hash in config and show pass":
(https://github.com/bitcoin/bitcoin/pull/27588#issuecomment-1540536920)
> From CI:
>
> ```shell
> 102/264 -
> rpc_users.py
> failed, Duration: 1 s
> stdout:
> 2023-05-06T19:48:12.787000Z TestFramework (INFO): PRNG seed is: 4992424049312590578
> 2023-05-06T19:48:12.806000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230506_193750/rpc_users_159
> 2023-05-06T19:48:13.197000Z TestFramework (ERROR): Unexpected exception caught during testing
> Traceback (most recent call last):
> File "/
...

👍 brunoerg approved a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1419107706)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2

Strongly agree on adding @ryanofsky on it.

👋 hebasto's pull request is ready for review: "Enable HW-accelerated implementations of SHA256 for MSVC builds"
(https://github.com/bitcoin/bitcoin/pull/24773)

💬 hebasto commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1540583507)
Rebased on top of the merged https://github.com/bitcoin/bitcoin/pull/27575 and undrafted.

💬 mzumsande commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1187910876)
This is a slightly different approach to `-fastprune` and others, which are set in `ApplyArgsManOptions()`.
So I wonder what is the criterion for an arg to be set in `ApplyArgsManOptions()`, as opposed to being initialized directly? And, closely connected, isn't the current approach of not calling `ApplyArgsManOptions()` in some spots (`setup_common`, some unit tests) a bit brittle? (considering that users could specify extra bitcoind args while running unit tests, which then would be ignored)

📝 sdaftuar opened a pull request: "p2p: Avoid prematurely clearing download state for other peers"
(https://github.com/bitcoin/bitcoin/pull/27608)
Avoid letting one peer send us data that clears out the download request (and related timers etc) from another peer.

The exception is if a block is definitely stored to disk, in which case we'll clear the download state (just as we do for compact blocks).

🤔 theStack reviewed a pull request: "Implement Mini version of BlockAssembler to calculate mining scores"
(https://github.com/bitcoin/bitcoin/pull/27021#pullrequestreview-1419015473)
Looks good to me overall, planning to do another review round within the next days.

For the `miniminer_overlap` test, I crafted some diagram and (ancestor) fee-rate calculation notes to get a better picture what's going on there in the mempool; will just dump it here, maybe it helps other reviewers or it even makes sense to include the diagram in a follow-up.

```
Tx graph for `miniminer_overlap` unit test:

coinbase_tx [mined] ... block-chain
---------------------------------
...

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1188846506)
nit: unused includes
```suggestion
#include <util/check.h>
```