🤔 darosior reviewed a pull request: "wallet: fix crash during migration due to invalid multisig descriptors"
(https://github.com/bitcoin/bitcoin/pull/31378#pullrequestreview-2462972517)
Ruling out consensus-invalid scripts makes sense, but discarding non-standard scripts is a much bigger leap. Also it would be a wack-a-mole: does the migration crash on any non-standard script? How much of that do you want to cover? Is it not possible to make the migration not crash on valid but not standard scripts instead of changing `IsMine` for those?

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1859491491)
Hmm, okay. I think it's better to remove it from this PR and maybe address the subject in another issue so it doesn’t interfere with the other commits.

So, the PR will include only:
- The --skip-missing-binaries option
- The error prompt to inform the user if no binaries are found in the build directory path.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2502387159)
Pushed a [small fix](https://github.com/bitcoin/bitcoin/compare/1ca5f7b998c14c44b6afd5e362fc4c007d8eebd3..0667ee2e5bbaba1d2965ab95644bdb27f881ffa1) addressing `blockhash` nullability.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1859531317)
Fixed.

👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2463413227)
Code review and light retest ACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21

The changes are very scoped to address the comment. Nice use of `get()`.

Repeated test in https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2450158177 for sanity.

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2502698185)
Rebased 2f9c2f4c6dcf8c45514e99d1018301aba61940fc -> 1f18eefe1e5bc3a97f5f6c9637a3a193e2c2f22e ([`pr/mine.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.12) -> [`pr/mine.13`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.13), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.12-rebase..pr/mine.13)) fixing silent merge conflicts and adding a functional test and updating gen-manpages

---

re: https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2394194530
r
...

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1783158715)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r1782379621

It would be surprising if current code couldn't hit this condition at all, though not surprising if this condition is just hard to hit because the tip is set to the genesis block very quickly on startup. Maybe could experiment with starting the node with reindex options or calling bitcoin-mine in a loop if it's worth debugging.

It would seem reasonable to not return optional though if getTip will not be available to
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860025340)
I mean the delay specifically, not the set. If we talk about an abstract "reconciliation set" (which is a combination of two), you apply the delay for an addition, but not for the removal action.

💬 maflcko commented on pull request "ci: allow passing extra args to docker build with CI_IMAGE_BUILD_EXTRA_ARGS":
(https://github.com/bitcoin/bitcoin/pull/31377#issuecomment-2503135628)
> The current CI runners don't need this, as they aren't ephemeral.

Currently, each runner is their own user account, with their own podman image layer cache, so in theory, they'd benefit from this as well.

However, the images are quite large (especially msan), so my worry would be that after a sufficient number of builds, the workers will run out of storage, as the cache isn't cleared like the local images are.

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

this could be moved into the lambda :)

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

Mind handling the result of erasure here? Either `Assert/Assume` or `if`

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860061020)
```
std::multimap<uint16_t, NodeId> parents_by_peer;
for (const auto &[peer_id, state_or_salt]: m_states) {
if (const auto state = std::get_if<TxReconciliationState>(&state_or_salt)) {
const size_t parent_count = std::count_if(parents.begin(), parents.end(),
[=](const auto& wtxid){return state->m_local_set.find(wtxid) != state->m_local_set.end();});
parents_by_peer.insert(std::make_pair(parent_count, peer_id
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860131262)
(this could also be done in 4b4d99fb2df2c60a2214487cec627bc560f50f53 `GetFanoutTargets`, although it's not that much an improvement over there)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860064472)
Multimap is ordered by key, and for same-key (same `parent_count`) since c++11 the order is consistent ([documented here](https://stackoverflow.com/a/13337612)). Feel free to include this as a comment, although I'm not even sure we care about the same exact output of multiple `SortPeersByFewestParents` calls.

(also it depends on the consistent ordering of `m_states` iterator too, which could be documented along the same lines, here and elsewhere it is iterated over)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860143301)
Yeah, and perhaps good time to squash this into eb2b19d2c237f2cc7b4ff3e7cefec52a37eb24b3, it got much stale by now.

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

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1860154761)
The issue is re-defining `wtxid` (for `parent_wtxid`) while it's already defined as a transaction-in-question :)
That was the source in my confusion originally

💬 laanwj commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1860193767)
Yes, it's unrelated to the scope of the PR anyway.

👍 laanwj approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2464281216)
re-ACK ee6185372fc317d3948690997117e42f6b79a5ff

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1860200766)
My preference would be to drop this comment blob. Otherwise, there will be bike-shedding when it is fine to remove it. Also, the code has to be touched again at that time. It seems better to just touch it once and then be done with it.

Suggested diff:


```diff
diff --git a/src/test/result_tests.cpp b/src/test/result_tests.cpp
index 6a23a7b..5e18d23 100644
--- a/src/test/result_tests.cpp
+++ b/src/test/result_tests.cpp
@@ -63,7 +63,8 @@ void ExpectSuccess(const util::Result<T>& result
...