💬 mzumsande commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2379597880)
Concept ACK

> In #29618 and here, I don't see the motivation to forbid v1 connections.

Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection. You could also run tor-only in this case, but that has its own disadvantages and `-onlynet=onion` shares the exact same downside that if everyone did it, the network would split. If som
...

📝 ismaelsadeeq opened a pull request: "test: enable running independent functional test methods"
(https://github.com/bitcoin/bitcoin/pull/30991)
- Some test methods in the functional test framework are independent and do not require any prior context or setup in `run_test`.
- This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
- Using this option reduces the time you need to wait before the test you are interested in starts executing.
- The functionality added by this PR can be achieved manually by commenting out code, but
...

💬 stickies-v commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1778860008)
Is there a reason to set `check_ratio`? It doesn't seem necessary?

```suggestion
CTxMemPool::Options mempool_opts{};
```

🤔 stickies-v reviewed a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334139707)
Strong concept ACK!

🤔 l0rinc requested changes to a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2333963688)
I have concerns about the translations, using raw format instead of adjusting the validator, and I think we could sneak in a fix for unterminated positionals.

<details>
<summary>Patch</summary>

```patch
Index: src/bitcoin-cli.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
--- a/src/bitcoin-cli.cpp (revision 8845a566
...

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778758009)
is this bug still possible now? If it is, consider updating the `strprintf` example

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778754066)
nit: we could avoid mentioning compile-time twice:
```suggestion
- *Rationale*: Tinyformat handles string parameters.
Using a string literal or `constexpr const char*` ensures
format strings are validated at compile time.
```

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778767590)
I'm not sure I understand how this change is supposed to work.

`"Copyright (C) %i-%i"` can be translated by checking the exact string from https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoinstrings.cpp#L280, but how is `"Copyright (C) 2009-2024"` be supposed to have a translated counterpart?

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778855186)
Unrelated:
just realized that we should eventually add `"%1"` passes, since `while ('0' <= *it && *it <= '9') {` can go beyond the string and passed accidentally - but seems to fail in tinyformat, see: https://godbolt.org/z/nbTKnrqch.
Should I add a new PR for that of do you want to add that case here?

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778776885)
👍, should have been there all along

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778768954)
lol

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778783288)
for consistency, consider using strprintf here as well:
```suggestion
BOOST_CHECK_MESSAGE(ret == expected, strprintf("[%s] mismatching requestables", comment");
```

----


Would be cool if we could use `BOOST_CHECK_EQUAL_COLLECTIONS` instead, but may not be trivial to do, haven't investigated in detail.

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882137)
nit:
```suggestion
// Ensure invalid format strings don't throw at run-time
```

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778882653)
We could add the examples from `bitcoin-cli` here:
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_SpecificUsageTests)
{
// These format strings contain '*' and should skip validation
PassFmt<6>("<-> type net v mping ping send recv txn blk hb %*s%*s%*s ");
PassFmt<5>("%*s %-*s%s\n");
PassFmt<22>(
"%3s %6s %5s %2s%7s%7s%5s%5s%5s%5s %2s %*s%*s%*s%*i %*s %-*s%s\n");
PassFmt<2>(" ms ms sec sec min min %*
...

💬 0xB10C commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2379684081)
Played around with the `getorphantxs 1` output a bit regarding visualization. You can now plug your own `getorphantxs.json` (verbosity >=1) into this demo tool https://observablehq.com/d/fb8142e7eb7f98f3. Produces something like this based on the size and from-peers (color) of the orphans.


💬 theuni commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716)
To avoid a race, this needs to lock `kernel_notifications.m_tip_block_mutex` before calling notify. `(*node.shutdown_signal)()` doesn't necessarily need to happen under the lock, but a lock is required after the signal in order to synchronize. Otherwise the update could be missed and we could potentially wait forever.

This is currently an example of [this trap](https://www.modernescpp.com/index.php/c-core-guidelines-be-aware-of-the-traps-of-condition-variables/) (see the "An atomic predicate"
...

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2379691000)
Here's an initial sketch of making Sv2Connman a subclass of SockMan. The test gets through the handshake but fails later on, so I'll need to study it a bit more closely.

https://github.com/Sjors/bitcoin/pull/64

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778939114)
`"Copyright (C) %i-%i"` remains the translateable string both before and after this change. The only difference is that we use the `bilingual_str` `format` overload instead of the `const std::string&` one.

👍 theuni approved a pull request: "guix: Drop no longer needed `PATH` modification"
(https://github.com/bitcoin/bitcoin/pull/30989#pullrequestreview-2334276129)
utACK f1daa80521eccebe86af6ee6fa594edf40eaa676 since guix is happy.

> 2. I don't understand why "In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to the `PATH` environment variable," according to the description. Setting this seems indiscriminate, like a sledgehammer approach, something that would cause the guix build to behave differently from normal depends builds and lead to confusing issues like this one.

As @hebasto mentioned [here](https://github.com/bitcoin/bitco
...

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1778950526)
Done.