💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937492567)
> Should I PR the above branch?

Opened https://github.com/bitcoin/bitcoin/pull/29418

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1937538080)
Concept ACK

💬 maflcko commented on issue "Internal bug detected: Fee needed > fee paid":
(https://github.com/bitcoin/bitcoin/issues/29398#issuecomment-1937653514)
@epiccurious The issue is intermittent, because "Restarting node helped." (see the report)

💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1937657442)
> Any conceivable downsides to removing it?

I can't see one, apart from the change consuming time to review it.

✅ sdaftuar closed a pull request: "Enforce incentive compatibility for all RBF replacements"
(https://github.com/bitcoin/bitcoin/pull/26451)

💬 sdaftuar commented on pull request "Enforce incentive compatibility for all RBF replacements":
(https://github.com/bitcoin/bitcoin/pull/26451#issuecomment-1937672476)
This work has obviously been superseded by the cluster mempool proposal, which solves these problems in a much better way than anything contemplated here.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-1937766372)
@epiccurious, p2p encryption "solves" the spying from intermediate routers on clearnet (aka man-in-the-middle). Tor, I2P and CJDNS solve that too. While this PR uses only Tor and I2P it would solve that problem. But there is more - it will as well solve issues with spying bitcoin nodes.

📝 vasild opened a pull request: "log: deduplicate category names and improve logging.cpp"
(https://github.com/bitcoin/bitcoin/pull/29419)
The code in `logging.cpp` needs to:
* Get the category name given the flag (e.g. `BCLog::PRUNE` -> `"prune"`)
* Get the flag given the category name (e.g. `"prune"` -> `BCLog::PRUNE`)
* Get the list of category names sorted in alphabetical order

Achieve this by using the proper std containers. The result is
* less code (the diff of the first commit is +62 / -129)
* faster code (to linear search and no copy+sort)
* more maintainable code (the categories are no longer duplicated in `LogC
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1485605463)
Done in https://github.com/bitcoin/bitcoin/pull/29419, thanks for the suggestion!

📝 vasild opened a pull request: "test: extend the SOCKS5 Python proxy to actually connect to a destination"
(https://github.com/bitcoin/bitcoin/pull/29420)
If requested, make the SOCKS5 Python proxy redirect connections to a set of given destinations. Actually act as a real proxy, connecting the client to a destination, except that the destination is not what the client asked for.

This would enable us to "connect" to Tor addresses from the functional tests.

Plus a few other minor improvements in the test framework as individual commits.

---

These changes are part of https://github.com/bitcoin/bitcoin/pull/29415 but they make sense on th
...

📝 vasild opened a pull request: "net: make the list of known message types a compile time constant"
(https://github.com/bitcoin/bitcoin/pull/29421)
Turn the `std::vector` to `std::array` because it is cheaper and allows us to have the number of the messages as a compile time constant: `g_all_net_message_types.size()` which can be used in future code to build other `std::array`s with that size.

---

This change is part of https://github.com/bitcoin/bitcoin/pull/29418 but it makes sense on its own and would be good to have it, regardless of the fate of https://github.com/bitcoin/bitcoin/pull/29418. Also, if this is merged, that would red
...

👍 epiccurious approved a pull request: "log: Nuke error(...)"
(https://github.com/bitcoin/bitcoin/pull/29236#pullrequestreview-1874393601)
utACK fa1d4f07c36dda3c72fb328918bddd7de062ef96.

💬 epiccurious commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#discussion_r1485621343)
nit - Noticed this when reviewing the changes. Since we're already changing the file - should Line 357/358 be a `LogError` not a `LogPrintf`?

💬 epiccurious commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#discussion_r1485623720)
nit - Also noticed this when reviewing the changes. Since we're already changing the file - should Line 413/422 be a `LogError` not a `LogPrintf`?

💬 vasild commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1485626063)
`test/functional/test_runner.py` contains:

```py
'rpc_net.py',
'rpc_net.py --v2transport',
```

So, on `master`, when I execute `test_runner.py` it will run this test two times, once in v1 mode and once in v2 mode. What about if I run, with this PR, `test_runner.py --v2transport`? Would that execute `rpc_net.py` two times and both in v2 mode? If yes, then what about adding `--v1transport` option as well? Or maybe just change the above to just:

```py
'rpc_net.py',
```


...

🤔 vasild reviewed a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1874395839)
Approach ACK b9912e2df9767540ab7f53cb3ba37fd2e2491806

The code changes look good. Just the question below.

💬 vasild commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1485622195)
nit: in commit message of b9912e2df9767540ab7f53cb3ba37fd2e2491806 `test: enable v2 for python p2p depending on global --v2transport flag`:

* s/all connection/all connections/
* "before this test" - should this be "before this change"?

💬 vasild commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1937824083)
What about setting `preferred_net`

https://github.com/bitcoin/bitcoin/blob/7143d4388407ab3d12005e55a02d5e8f334e4dc9/src/net.cpp#L2682

if `-onlynet` is used? Surely it is a waste to select an unreachable address:

https://github.com/bitcoin/bitcoin/blob/7143d4388407ab3d12005e55a02d5e8f334e4dc9/src/net.cpp#L2695-L2697

That would skew the outcome though. If addrman has 60% IPv4, 25% Tor and 15% I2P addresses right now it is more likely to select Tor over I2P. If we run with `-onlynet=tor
...

⚠️ vostrnad opened an issue: "Assertion chainman().ActiveChain()[height] fails on startup on memory-constrained system"
(https://github.com/bitcoin/bitcoin/issues/29422)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

bitcoind crashes on startup with this error: (not logged in debug.log)
```
node/interfaces.cpp:525 getBlockHash: Assertion `chainman().ActiveChain()[height]' failed.
Aborted (core dumped)
```
This is after trying to bring the node up after a previous crash from which I don't have the error as I was running in `-daemon` mode. The previous crash happened during IBD at height 140920.

##
...

💬 satsie commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1937844734)
RE:

>> ... this requires the "inverse" ...
>
> Ha, I did not realize this before! I am fine either way.

I voted for `bitcoin-cli getnetmsgstats '["message_type"]'` because as a node operator it's simpler to remember the dimensions I want (i.e. message) instead of knowing the RPC well enough to list every dimension I don't want (i.e. direction, network, connection type). Of course anyone using an RPC should read the docs :smile: