📝 furszy opened a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757)
Before detailing the issue:
This crash should not occur in production, as block events are queued and processed in
order by the validation signal workers. This PR is just fixing brittle code.

Issue:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToW
...

📝 sr-gi opened a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758)
Some of the networking constants defined in p2p_tx_download.py are more generally defined in p2p.py

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience o
...

💬 darosior commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622618673)
I'm not sure i'm the most legitimate to give my opinion here, but since you asked me explicitly i will share.

First of all why is the default position that we should ship a new binary on short notice right before feature freeze, and people who are not comfortable with this should argue against it? It seems backward to me. The default should be business as usual and if people want to introduce new binaries for the project to release, they should be arguing for it.

If you start from this default
...

📝 sr-gi opened a pull request: "test: fixes p2p_ibd_txrelay wait time"
(https://github.com/bitcoin/bitcoin/pull/31759)
`p2p_ibd_txrelay` expects no GETDATA to have been received by a peer after announcing a transaction. The reason is that the node is doing IBD, so transaction requests are not replied to. However, the way this is checked is wrong, and the check will pass even if the node **was not** in IBD.

This is due to the mocktime not being properly initialized, so the check is always performed earlier than it should, making it impossible for the request to be there.

This can be checked by modifying the
...

🤔 hodlinator reviewed a pull request: "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`"
(https://github.com/bitcoin/bitcoin/pull/31734#pullrequestreview-2581415219)
Code review 0bc213db7686df9ccbd5ef76c5569181851d80a2

Thanks for taking on this issue! Not very familiar with this area but maybe I can assist in refining the PR anyway. Seems directionally correct to use `h` consistently for hardened derivation paths unless backwards compatibility is requested.

### PR title/summary

#### PR title typo
acount -> account

#### Summary

```diff
- In `MiniscriptDescriptor::ToStringHelper()` only `StringType::Private` variant of `type` argument was acco
...

💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934112560)
nit: Could keep style with rest of the fields:
```suggestion
const DescriptorImpl::StringType m_type;
```

💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934100346)
In commit 0bc213db7686df9ccbd5ef76c5569181851d80a2:
This change should be part of the other commit.

💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934103988)
Seems okay to store as raw pointer, if we take it as such as an argument into the `StringMaker` constructor?
```suggestion
const DescriptorCache* m_cache;
```

💬 hodlinator commented on pull request "miniscript: acount for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1934126766)
Should probably forward on the `cache` argument to the StringMaker.
```suggestion
if (const auto res = m_node->ToString(StringMaker{arg, m_pubkey_args, type, cache})) {
```

💬 ryanofsky commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622685379)
Just to clarify it sounds like your opinion has nothing to to with ipc or process separation a mining interface.
You are just opposed as a matter of principle to releasing any new set of binaries including any experimental feature? This is a new pov to me if I am understanding it correctly so thanks for expressing

📝 sr-gi opened a pull request: "test: make sure we are on sync with a peer before checking if they have sent a message"
(https://github.com/bitcoin/bitcoin/pull/31760)
p2p_orphan_handling checks whether a message has not been requested slightly too soon, making the check always succeed. This passes unnoticed since the expected result is for the message to not have been received, but it will make the test not catch a relevant change that should make it fail.

An easy way to check this is the case is to modify one of the test cases to force a request within the expected time, and check how the request is not seen. After the change, the test would crash as expe
...

💬 achow101 commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#issuecomment-2622754123)
ACK 551a09486c495e1a3cfc296eafdf95e914856bff

🚀 achow101 merged a pull request: "net: Disconnect message follow-ups to #28521"
(https://github.com/bitcoin/bitcoin/pull/31633)

💬 mzumsande commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934559681)
> without this PR's change, we may be doing the re-evaluation work up to num_inbound times erroneously?

yes - I suggested this because it seemed a bit wasteful in the honest case, but it is probably no big deal, because the actual full validation will only be done once and checking inputs repeatedly shouldn't be too much additional work.

On the other hand, I think that this attack would be really hard to pull off timing-wise (you would need to guess when other peers do orphan resolution, a
...

🤔 mzumsande reviewed a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666#pullrequestreview-2582142307)
Concept ACK, just one question / suggestion.

💬 mzumsande commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934526753)
I find the flow of logic a bit convoluted:

The check whether a peer is a orphan resolution candidate happens in `OrphanResolutionCandidate`, which also calculates the delay and is called from `AddOrphanParentAnnouncements`.

I would find the folllowing kind of of flow easier to understand (no need for `std::optional`, functions do exactly what their name suggests they do):
```
if (IsOrphanCandidate()) { // only checks if peer is a candidate
AddOrphanParentAnnouncements(); // delay is
...

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2622819631)
Sorry about the churn here. Went back to ee4c9d7eba6ba6fb6dcb7abd539c23f5b62d5991 and was able to reproduce the `TimeoutError.errno==None`-issue there too, which is a relief. It seemed to be a bit timing-sensitive, so in the latest push I increased the still low `rpc_timeout`-override and applied `self.options.timeout_factor` to it in hopes of making it more robust.

💬 achow101 commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2622831277)
~0

I've read through the conversation but I still don't understand why this change is beneficial?

This is also a major breaking change and would break a significant amount of external tooling that rely on the current location of the binaries.

🚀 achow101 merged a pull request: "test: improve BDB parser (handle internal/overflow pages, support all page sizes)"
(https://github.com/bitcoin/bitcoin/pull/30125)

✅ achow101 closed an issue: "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic"
(https://github.com/bitcoin/bitcoin/issues/31210)