🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2315917440)
re: https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2361045807

Thanks for the review!

> The only part that seems (slightly) more involved is in [00a82f4](https://github.com/bitcoin/bitcoin/commit/00a82f45004a887c2b58ed8da3db9280e0ec8b5c). If someone were to refactor `BlockValidationState`, they would have to also adjust its `CustomBuildMessage` and `CustomReadMessage` counterparts.

I think it might be good idea to change `testBlockValidity` method anyway to return validation
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1767053902)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1766639028

> [974e041](https://github.com/bitcoin/bitcoin/commit/974e041b794072395e41701cd7a71ebdcc362e48): `serialize.h` already defines a `concept Unserializable` and `Serializable `, introduced by @maflcko in #29056.

This is interesting. The concepts are not interchangeable because the definitions in #29056 check whether a particular class can be serialized and unserialized with a particular stream class, while the concepts h
...

💬 kevkevinpal commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361475900)
was there a seed value for https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2357565564?

I'm trying to look for it to try and reproduce but am having trouble finding it

👍 theuni approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2316048259)
utACK 7025942687fd5e91d0a10ce5b2ac673b67a63491

📝 jonatack opened a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930)
Been using this since May 2023.

- add a peer services column (considered displaying the p2p_v2 flag as "p" or "2"; proposing "2" here for continuity with the "v" column)
- remove the "v" transport protocol column, as that info is provided in the new peer services column
- clarify in the help that "relaytxes" and "addr_relay_enabled" are from getpeerinfo
- add a level 5 for an outbound-only peer list. Several people have requested this, to keep the output within screen limits when running n
...

💬 brunoerg commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2361513087)
Just provided the fuzz inputs for testing: https://github.com/brunoerg/qa-assets/pull/1

💬 sipa commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361528887)
It's possible to be connected to a peer that supports the v2 service using the v1 protocol.

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1767171275)
Agree it can wait for a followup, but if you have to retouch it would be good to add a comment.

💬 maflcko commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361566377)
> I'm trying to look for it to try and reproduce but am having trouble finding it

I think the logs were nuked in the meantime, which is a Cirrus bug :(

IIRC it may have been a shutdown error of the v0.16.3 software. However, I don't think looking at the CI failure matters, because the test doesn't make much sense and a more realistic test already exists.

💬 maflcko commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361584037)
cc @Sjors from 89a28e02fa46f3d5eb07ab02aa34aa95c6fcee11

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361591097)
> It's possible to be connected to a peer that supports the v2 service using the v1 protocol.

Thanks, am I missing something? When calling it on a node configured as `v2transport=0`:

```
Bitcoin Core client v28.99.0-18fcca480165 - server 70016/Satoshi:28.99.0(jon)/

<-> type net serv mping ping send recv txn blk hb addrp addrl age asmap id version
in onion nwl2 487 487 13 0 1 0 14 70016/Satoshi:27.0.0/
in oni
...

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361608191)
(The proposal currently only returns service flags that are present. Could optionally add a "1" if the p2p_v2 flag is unset.)

💬 sipa commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361621928)
I meant this as a comment on:

> remove the "v" transport protocol column, as that info is provided in the new peer services column

They represent different information. The "v" transport protocol column currently conveys whether or not you are *right now* speaking to that peer through the v1 or v2 protocol.

Peer services are about what a peer supports, not what protocol you're speaking with it.

If you run with -v2transport=0, the transport protocol will be v1 with all peers, includes ones th
...

👍 tdb3 approved a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2316167311)
ACK dcd66472600f1676e2e450245924ab6e1a837d39

Nice addition. Compliments `wait_until` nicely.

(As expected) saw little difference in both individual and total runtimes of tests on master (141s) vs pr branch (141s), (at least within typical run-to-run difference).

### Master (e46bebb444dfff89813333dc1926a57c9a3e8aab):
| test | status | duration(seconds) |
|---|---|---|
| feature_assumeutxo.py | Passed | 32 |
| feature_minchainwork.py | Passed | 10 |
| mempool_unbroadcast.py | Passed
...

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361627003)
Ah! Thanks.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361652277)
Ah! Indeed, thank you.

```
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id
in onion 1 * . 0 95
in onion 1 368 556 17 37 * . 6 68
in onion nwl2 1 376 551 0 1 0 30 13 14
in onion nwl2 1 380 430 0 3 24 12 38
...

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2361663097)
@maflcko thanks for the review! Your comments make sense to me, I'll address them in the followup.

I just added some inputs for this target to qa-assets. Here is a [coverage report](https://marcofleon.github.io/coverage/p2p_headers_presync/).

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361685482)
@sipa make it make sense to only return the "v" column if the server node is `-v2transport=0`?

💬 sipa commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361691952)
@jonatack I don't think so. It's possible that the service flag and transport type differ even with `-v2transport=1`.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361715093)
@sipa Thank you. Dropped the commit that removed the "v" column.

💬 Sjors commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361722871)
Since we can longer create a legacy (bdb4) wallets and since older Bitcoin Core versions can't load descriptor (sqlite3) wallets, it should be safe to drop certain older nodes from the _downgrade_ tests.

Will study this PR later.