💬 maflcko commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#issuecomment-2883537760)
lgtm ACK 58e55b17e632dbd4425dd64825b087f242ac4b7b

💬 sipa commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#discussion_r2090994342)
Strange, GitHub renders this line in the diff view as indended by 4 spaces compared to the line above. I don't see anything strange in the code itself, though.

💬 sipa commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#issuecomment-2883546999)
Concept ACK

📝 hodlinator opened a pull request: "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/32509)
* Handle multiple `--tmpdir` args properly.
* Handle `--timeout-factor=0` properly (fixes #32506).
* Improve readability of expected error message (`assert_raises_message()`).
* Make suppressed error output less confusing (`wait_for_rpc_connection()`).

💬 hodlinator commented on issue "test: feature_framework_startup_failures.py fails with `--timeout-factor=0`":
(https://github.com/bitcoin/bitcoin/issues/32506#issuecomment-2883569700)
Proposed fix in #32509.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090831336)
In 9304257d67e6f280cc1b45021118c6bb37489d89:

> Since we don't remove blocks after inserting into
setBlockIndexCandidates anymore, this means that blocks could be inserted
multiple times now into the set - this won't have any effect, so
behavior isn't changed.

I suspect with "remove blocks" you mean erasing from `highpow_outofchain_headers`? In which case, I don't think this part of the commit message is correct, at least for this commit?

👍 stickies-v approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2843060953)
ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4

I've thoroughly gone through this PR many times now, and I don't think I can assure myself anymore than I am now that it is correct and safe. Still, the code touched in this PR is fairly complicated and coupled, and I am not as familiar with this validation logic as others, so my main concern is that there are unexpected side-effects I've not considered.

Nits can be ignored, comments (if correct) can be addressed in other pulls.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090791378)
This code block now duplicates `InvalidBlockFound()`, minus the `setBlockIndex` erasure - but I think that should be a (fast) no-op, perhaps it's worth deduplicating this?

<details>
<summary>git diff on ed172d3002</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 31a0f54c11..3312849024 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4516,11 +4516,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,

if (
...

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090976170)
nit: developer notes suggest line length < 100
```suggestion
const bool best_header_needs_update{
m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip
};
if (best_header_needs_update) {
// pprev is definitely still valid at this point, but there may be better ones
m_chainman.m_best_header = invalid_walk_tip->pprev;
}
```

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090892085)
grammar nit: parallel structures (and line-length)
```suggestion
// If invalidated, the block is irrelevant for setBlockIndexCandidates and for
// m_best_header so it can be removed from the cache
```

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090806322)
nit
```suggestion
highpow_outofchain_headers.insert({candidate->nChainWork, candidate});
```

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090961175)
note: this logic is different to `RecalculateBestHeader`: https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/src/validation.cpp#L6404

From our [earlier conversation](https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2059128406) it seems like `CBlockIndexWorkComparator` is the correct choice here, so probably this can be fixed (if necessary) in a separate pull, but it seems worth raising here? Relatedly, I tried modifying `CheckBlockIndex` to use `CBlockIn
...

📝 Sjors opened a pull request: "rfc: only put replaced txs in extra pool"
(https://github.com/bitcoin/bitcoin/pull/32510)
We currently store rejected mempool transactions in the extra pool. This includes (some) policy and consensus rejected transactions.

It seems trivial to attack this pool for free with consensus invalid transactions, or almost free with policy violating transactions that are extremely unlikely to ever get mined.

At the same time the orphanage already handles some cases that were initially dealt with here.

This commit simplifies things by only putting replaced transactions in the extra po
...

💬 Sjors commented on pull request "rfc: only put replaced txs in extra pool":
(https://github.com/bitcoin/bitcoin/pull/32510#discussion_r2091044581)
Not sure what to replace this check with, but now it doesn't check anything.

💬 brunoerg commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#discussion_r2091049875)
Yes, this is weird. I don't know why.

💬 maflcko commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#discussion_r2091058887)
The other lines in this test are excessively long. It would be better to use c-style multi-line string:

```c
CheckUnparsable("multi("
"bla,"
"foo,"
);
```

However, this seems unrelated to the changes here and would require reformatting the whole file.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2843363337)
Updated 4e1aae19512df82af584a064640c2143c5c5fa4f -> 7af6e1089ea264e870b26ac83e81e7aa374acbe1 ([`pr/wrap.32`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.32) -> [`pr/wrap.33`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.33), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.32..pr/wrap.33)) with suggestions adding windows uninstall line, removing no longer used cmake build prefixes, and improving many comments.

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091049127)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087629089

Thanks, added a code comment with this information

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091019509)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2087583779

Just added a note to the `GetExePath` doc, because I'm not really sure `std::nullopt` is better representation of a path that could not be determined than `path.empty()`. Returning {empty path, nonempty path} seems safer than returning {null path, empty path, nonempty path} if the goal is to avoid bugs.

Theoretically if we had a `not_empty<T>` wrapper which asserted the wrapped value is non-empty on construction maybe
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2090970340)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2086894554

Good catch, fixed!