💬 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!

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

Thanks, added a similar comment

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

I think they are somewhat distinct. The subprocess header is trying to mimic python's subprocess API and provides functions for starting and interacting with child processes. This header provides functions for getting information about and replacing the current process that don't have any equivalent in python's subprocess API. So not sure I'd want to merge them, though it could be a possibility.

For copyright probably
...

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

Good catch, added.

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

> are these prepended paths (here and also for the `gui`, `bench` and `test` commands above) still relevant? They seem to reflect a folder structure that doesn't exist anymore since #31161

Good find, removed these. The prefixes were harmless but no longer do anything after the update in https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2819431273 following the #31161 merge.

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

Thanks to both for the suggestion and testing. Added this line