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

💬 naumenkogs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1841700796)
Have you left a comment on my branch? I can't see it for some reason.

💬 naumenkogs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2475640512)
@sdaftuar

>EDIT: I hacked up the validation logic to trigger this currently unreachable block of code -- ie to make the ConsensusScriptChecks() fail in a package validation context -- and verified that logic to remove the newly added transactions seems to work. Would be great for others to verify this as well!

I had a little doubt that it will remove the transactions... Instead I was worried about evictions (i can't think of other side effects) of add/remove invalid stuff. Doesn't this a
...

👍 rkrux approved a pull request: "test: enable running independent functional test sub-tests"
(https://github.com/bitcoin/bitcoin/pull/30991#pullrequestreview-2435413648)
reACK 409d0d629378c3e23388ed31516376ad1ae536b5

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1841802108)
If I'm not missing something, why not just remove it?

💬 rkrux commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1841810405)
> if no binaries are found in /build

This could also be because the user might not have built the binaries yet even in the case of default build path.

A more verbose message could be:
```
'No binaries found in {builddir}. Please ensure the binaries are present in {builddir}, or set another build path using the BUILDDIR env variable.'
```

💬 naumenkogs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1841755721)
7dc8a0ba1be86352fa51860af504ed09fba9307e

What does `valid` mean in this context? Can't this word be dropped?

💬 naumenkogs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1841816179)
f306a940d2ba470a13120853ace49509a8e496a6

looks odd, perhaps a pasting issue.

💬 naumenkogs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1841774691)
7020d29e7fb276af9877b6767b4bacf1edfe050c

wanna also check for non-standard if add one more?