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

💬 darosior commented on issue "RFC: multiprocess binaries in 29.0":
(https://github.com/bitcoin/bitcoin/issues/31756#issuecomment-2622845955)
Yes, i would say the same about another feature in the same stage of development.

💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2622853846)
@achow101 This is one of the changes I was referring to in last week's IRC meeting. We'd rather not move these around after v29 (with the exception of some moves to libexec) so that nobody has to migrate to CMake and then adapt again.

Not sure what you mean about breaking existing tooling though? This affects the location in the CMake build tree. It's not related to installation locations. So afaik the only tooling that needs to be updated has been changed in this PR. Presumably anyone else r
...

💬 achow101 commented on pull request "RPC: improve SFFO arg parsing, error catching and coverage":
(https://github.com/bitcoin/bitcoin/pull/30844#issuecomment-2622885360)
ACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e

💬 glozow commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934672284)
I wonder if a good "mitigation" (if we're worried) is @mzumsande other suggestion, which is to process 1 thing off the peer's workset when it connects? Although if trying to "attack" this way, the solution then is to just have 2 in the workset. I'd propose the same trick then, which is randomly popping from workset. That outta be good enough? I prefer something like this to potentially re-validating repeatedly.

However I think it's very realistic to add outbound/inbound information to txorpha
...