💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841164936)
`2==true` would mean that we had the transaction in both places, which should never happen (I'd mean that a transaction is both ready and delayed at the same time).

If that were to happen in production for whatever reason, this would return false with the suggested changes, which may break something?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841223757)
In that case, no transaction would be moved from the reconciliation set to `invs_to_send`, given no transaction would be removed:

```cpp
auto it = std::find(fanout_with_ancestors.begin(), fanout_with_ancestors.end(), peer_id);
if (it != fanout_with_ancestors.end()) {
for (const auto wtxid: parents) {
m_txreconciliation->TryRemovingFromSet(peer_id, wtxid);
invs_to_send.push_back(wtxid);
}
} else {
// If the peer is registered for set reconciliation, maybe pi
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841243963)
Done

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841245788)
Done

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841245945)
Done

👍 hodlinator approved a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2434652082)
re-ACK fe39acf88ff552bfc4a502c99774375b91824bb1

`git range-diff master ecc5cb9 fe39acf`

- Added more `FailFmtWithError`-tests ([maflcko](https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2423906139)).
- Terser skipping of `%%` ([l0rinc](https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1835770569)).
- Comment regarding format string components updated ([me](https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1834472950)).

💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1841318869)
Thanks for clarifying when modified fees are ignored.

> We're assuming node operators/miners care about dust. If they do not, they can already set their dust relay rate to 0 and avoid all of these mechanisms entirely, ephemeral dust becomes a no-op. (e.g., MARA reportedly doesn't enforce dust limits and runs an accelerator already)
>
> If we end up being wrong and miners are ok putting one dust output in the utxo set but not multiple or whatever, we can revisit?

My assumption would be th
...

💬 achow101 commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2475050409)
ACK fa66e0887ca1a1445d8b18ba1fadb12b2d911048

🤔 hodlinator reviewed a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2434704657)
I've experimented with a change on top of this PR that removes `shared_ptr` usage from *src/script/* altogether in order to clarify ownership.

My change is in 80fca845b5f28677207a8fea4a173baaef23036f. It helped uncover another place where the `node` needs to be cloned (at the bottom of the outer loop):

```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index b099fdfc3f..f0294df72a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -2154,7
...

🚀 achow101 merged a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000)

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2475109762)
> My change is in [80fca84](https://github.com/bitcoin/bitcoin/commit/80fca845b5f28677207a8fea4a173baaef23036f). It helped uncover another place where the `node` needs to be cloned (at the bottom of the outer loop):

Good catch, added that.

Do you have a particular test case that triggered there?

👍 ajtowns approved a pull request: "validation: Remove RECENT_CONSENSUS_CHANGE validation result"
(https://github.com/bitcoin/bitcoin/pull/31269#pullrequestreview-2434890591)
ACK e80e4c6ff91e27d7d40f099a2d7942c29085234c

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1841534621)
Good suggestion. Thanks!

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1841572082)
The most common use case is running the script without setting BUILDDIR, in which case the default builddir, `/build`, is used. However, if no binaries are found in `/build` we remind the user to set BUILDDIR to specify the correct path.

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2475531811)
Rebased and squashed commits.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841676251)
Say there is an in-mempool parent announced earlier, so it's not in any of the sets.
`SortPeersByFewestParents` will return the same order as `m_states` (since all `parent_count` are 0),
and `fanout_with_ancestors` will take first two after resize.

Then for two peers (specifically, the first two from `m_states`) this code is executed:
```
for (const auto wtxid: parents) {
m_txreconciliation->TryRemovingFromSet(peer_id, wtxid);
invs
...

💬 0xB10C commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2475583549)
fwiw: https://x.com/rodarmor/status/1856879301578428826

> I did a little hacking on Bitcoin Core and wrote a new RPC command to export JSON describing the entirety of the RPC interface.
> If you're writing a RPC client for Core, using this with a code generator is definitely the way.
> https://github.com/casey/bitcoin/blob/rpc-json/api.json

cc @casey

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

First I thought why don't `fanout=true` here and handle it below. We can't do it because txid/wtxid difference (although we could have a `txid_or_wtxid` variable above....). But then, you certainly want to at least set `fanout=false` here, otherwise it remains true and added to `invs_to_send` twice (it's not critical but a bug still).

💬 maflcko commented on issue "Unit test failures when using multiple jobs and RANDOM_CTX_SEED":
(https://github.com/bitcoin/bitcoin/issues/30696#issuecomment-2475616303)
This was fixed in ad9c2cceda9cd893c0f754e49f7fca6e417ee95f by using the test name as seed.

At least I can no longer reproduce, even when pinning the time to a constant:


```diff
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 46a6daa88c..185adf3457 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -138,7 +138,7 @@ BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)

const std::stri
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1841697846)
Probably a taste thing. I think `auto` should be used only when it's obvious what type it is.

Here there are two non-obvious occasions:
- `auto x = bool + bool` resulting in x=2.
- casting 2 to bool for the result.

I won't insist but i think verbose types are better here. Feel free to close.