💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767050387)
It is indeed, thanks for pointing it out. Resolved by dropping the first commit and applying a minimal diff to `netbase.cpp`.

💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547)
Not sure about this. `tfm::format` is the only remaining function preventing the removal of `./test/lint/lint-format-strings.py`. I wanted to do this in a follow-up, but if this function is renamed, it will be harder, because all call sites will have to be renamed.

My preference would be to leave the name as-is and instead just delete the unchecked one, or rename that one to `format_maybe_throw` (or similar).

If you don't mind, it would be good to delete `./test/lint/lint-format-strings.py
...

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054801)
I don't agree it's redundant:
- it's not obvious from the tests that we're not just checking error messages, but that we're ensuring no errors are thrown, which is the difference with `tfm::format()`
- additional tests to try_format may be added in the future that test other aspects, so this acts as a little delineator

💬 mzumsande commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2361367099)
Though one could also make a case for not allowing importdescriptor/rescan during background sync, with the reasoning "just wait a little bit longer" that doesn't really apply to the pruning case. Would be interesting to know if this has ever been discussed for assumeutxo.

🚀 fanquake merged a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927)

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2361384689)
Thanks for the reviews! Added a commit renaming the file build.h to bitcoin-build-info.h, to account for both the namespacing suggestions (https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2310630029) and the request for a more descriptive name (https://github.com/bitcoin/bitcoin/pull/30856#discussion_r1764891407). Let me know if another name is preferred (dashes vs. underscores? put "git" into filename or not?) and if a scripted-diff for moving bitcoin-config.h is preferred in thi
...

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#discussion_r1767071727)
> Now that HAVE_BUILD_INFO is removed, this can be placed under the "normal" includes, no?

I left it where it is for now, as I thought it make sense if the comments and the preprocessor commands evaluating `BUILD_GIT_TAG` are immediately below, but no hard feelings. Happy to move only the include up if that is preferred.

👍 maflcko approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2315960896)
lgtm

Maybe squash, to avoid changing the file name twice?

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2361420302)
> Maybe squash, to avoid changing the file name twice?

Makes sense yeah, done.

🤔 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
...