💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820533885)
The following test would break the code if we forget the double cast (discussion [here](https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)). I suggest adding it to the tests.

```
std::tie(in_fanout_targets, out_fanout_targets) = tracker.GetFanoutTargets(Wtxid::FromUint256(frc.rand256()), /*inbounds_fanout_tx_relay=*/0, /*outbounds_fanout_tx_relay=*/100);
BOOST_CHECK(!tracker.ShouldFanoutTo(peer_id0, in_fanout_targets, out_fanout_targets));

```

💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443832515)
> as in given that context

I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.

practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820538647)
(technically the code you have is correct, it's just confusing for no reason i think)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820542894)
4234f5ebe133b949080aaa56da8cbdd18b650ff2

We can just merge the two vectors, no? There is no use for this separation.

💬 jarolrod commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443840863)
> > as in given that context
>
> I see. No I don't think they would have been useful in that context, because all the base58 benchmarks run with inputs of the same size, so they couldn't have found that particular issue and therefore were also not useful in verifying the fix.
>
> practicalswift mentioned a fuzz test in the PR, which is likely how he found the issue (speculating).

I'm not intending to argue, and as i said your main point in the PR is valid.

If a bench didn't exist at
...

👍 maflcko approved a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2401315598)
Nice. This looks less scary than expected. Left a nit to add more compile time checks, but this looks good either way.

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820457426)
nit in 97dd5fe5128592332c83998825bbeda063815120: in the commit message: Missing `needed *in the* next commit`.

Also, it would be good to add some failing test cases for this stuff.

For example, `"%1"` should fail, because `terminate called after throwing an instance of 'tinyformat::format_error'
what(): tinyformat: Conversion spec incorrectly terminated by end of string`. See https://godbolt.org/z/1eehh1oTv . Also see 51c56e8b38033b96fb3930c2bcba3add2047d324

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820504400)
Also, some more "fancy" passing test cases:

* `PassFmt<3>("'%1$- 0+*3$.*2$f'");` (space and `0` are ignored, confusing, but valid)
* `PassFmt<3>("'%- 0+*.*f'");` (same, without pos specs)

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820461851)
Other failing test cases to possibly add: `"%*"`, `"%+*"`, `"%.*"`, ...

💬 maflcko commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820543443)
Nit in 1d16d6e6bac994fed7c695f530b9984edcd290bd: I don't really see why this is useful. Maybe I am missing something, but everything compiles by just moving the two members to the derived struct:


```diff
diff --git a/src/tinyformat.h b/src/tinyformat.h
index 56c25d8f7f..6227cdeca0 100644
--- a/src/tinyformat.h
+++ b/src/tinyformat.h
@@ -179,21 +179,16 @@ namespace tfm = tinyformat;

namespace tinyformat {

-// Added for Bitcoin Core. Wrapper type for format strings to allow comp
...

💬 laanwj commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1820550576)
By adding this if here, this no longer accounts for the situation where port mapping is turned off. i think i slightly prefer the old API.

💬 laanwj commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1820574292)
Was this verbose code that is replaced here a workaround for lack of `try_emplace`?

📝 TheCharlatan opened a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175)
With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden.

The rpc interface for ProcessNewBlock (su
...

✅ hodlinator closed a pull request: "Windows bitcoind stall debugging [NOMERGE, DRAFT]"
(https://github.com/bitcoin/bitcoin/pull/30956)

💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2443920476)
Alright, our time is finite and hopefully `CryptGenRandom` should be enough for now. Closing.

👍 rkrux approved a pull request: "test: Don't enforce BIP94 on regtest unless specified by arg"
(https://github.com/bitcoin/bitcoin/pull/31156#pullrequestreview-2401473616)
tACK e60cecc8115d3b28be076792baa5e4ea26d353a6

Successful make and functional tests. Agree with the approach to enable BIP94 on regtest through the test argument.

💬 rkrux commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#discussion_r1820557540)
Worth creating a directory `doc/release-notes-prs` and putting it there?
I see there's a `release-notes` directory that contains notes for the releases.

💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1820609170)
Yeah i agree, was confused initially. Please resolve.

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1820658956)
However, as the file is taken from upstream, I wonder how much it should be modified at all.

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1820669760)
hmmm, I'll check it out, I wasn't aware that's how this works (fuzzing is currently completely broken for me locally), but this sounds like a huge blind-spot for fuzzing - like you said, we should investigate, false confidence in this area is dangerous.

💬 fanquake commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2444030316)
I wonder if a better approach here would be to further integrate the manpage generation into the build system; given that the two issues of "which binaries exist" and "where do they exist" are already solved in that case. This should also remove the dep on `git`.