💬 ismaelsadeeq commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1688246363)
Why is this false, shouldn't this be true also?

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1688376618)
since they're always equal, I suspect they can never be unequal? Perhaps I'm violating some C++ custom here?

🤔 glozow reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2191951101)
ACK cad318fa843

I did code review to convince myself that the code does what it's supposed to be doing / what's described in the delving post. Everything is a bit abstract so I manually tested by writing some naive versions of `BlockAssembler` and `MiniMiner` using cluster_linearize. I also code reviewed the tests and introduced bugs to check their coverage. I didn't really dive into `DepGraphFormatter` but it seems to work.

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688140732)
I suppose this is the same as `chunking.Intersect(intersect)` except without exiting early?

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1686776442)
2597f096afde07d9c7f6466c0970a73947562be3

nit: found this comment hard to parse. "Remove transactions from the chunking object, and check various..." ?

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688134395)
There wouldn't be any problem with resetting `subset = SetInfo(depgraph, ReadTopologicalSet(depgraph, todo, reader));` here, right?

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1687853218)
Question: I whiteboarded what this graph + the decision tree looks like to try to understand what makes this a hard graph. Do I understand correctly that
- Within this PR, the feerates of transactions in the graph have no impact on how many iterations `SearchCandidateFinder` might run, since all work items are considered as long as they are topological.
- The feerates of the parent+child transactions {4..n-1} become important after #30286's optimizations that rule out transactions/branches tha
...

💬 Sjors commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2245733622)
> The simplest way to do that might just be to explicitly hardcode the block hash of the alternate chain which forks from ours as invalid, which would immediately address both of these additional problems in addition to making assumeutxo work in this scenario.

That's effectively a (negative) checkpoint. Hopefully this heaver chain is invalid in such a way that we can detect it without context. E.g. perhaps it contains a 4.1MB block block (with less work that the real tip). In that case we wou
...

💬 Sjors commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2245743124)
Post merge concept ACK

💬 Sjors commented on pull request "rpc: Return errors in loadtxoutset that currently go to logs":
(https://github.com/bitcoin/bitcoin/pull/30497#issuecomment-2245749356)
Concept ACK

🤔 theuni reviewed a pull request: "cleanse: switch to SecureZeroMemory for Windows cross-compile"
(https://github.com/bitcoin/bitcoin/pull/26950#pullrequestreview-2194539139)
Concept ACK. Can't think of any reason why not, if mingw supports it.

💬 ismaelsadeeq commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1688412840)
Oh yes since all instances are equal, no instance is less than another, so should indeed be `false`.
maybe add a comment on that.

👍 ryanofsky approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2194357807)
Code review ACK 09ce3501fa2ea2885a857e380eddb74605f7038c. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome.

It seems like may be some suggestions from paplorinc that could be responded to, but they are pretty minor. So this looks ready for merge.

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688288830)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686711284

> it's not very important in this particular method, but `inline` + `const` could help with the actual inlining - it's also clearer to the reader to understand what happens when mutating the parameter, since after inlining it should ideally avoid the copying (which is easy to do for a const). At least this was my thinking for recommending it, let me know if I'm mistaken.

Responded in the other thread https://github.co
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1688337546)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1686699872

> so after the call its internal state is different.

I may not be understanding the claim correctly, but just to be clear, the string_view argument is passed by value (not by reference) so it can't be changed by the call.

From the callers point of view there is no difference between:

```c++
void ProcessString(std::string_view str);
void ProcessString(const std::string_view str);
```

Either way, whatever ar
...

💬 achow101 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2245824190)
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1688444784)
This can fail intermittently:

```
node0 2024-07-22T16:31:54.104994Z [httpworker.0] [rpc/request.cpp:232] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
test 2024-07-22T16:31:54.291000Z TestFramework (INFO): Sending first 4 bytes of ellswift which match network magic
test 2024-07-22T16:31:54.292000Z TestFramework (INFO): If a response is received, assertion failure would happen in our custom data_received() function
test 2024-07-22T16:31:54.292000Z TestFramework
...

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688448049)
Yes, the current `SearchCandidateFinder` is this PR effectively searches over *every* topologically valid subset of the cluster (if the `max_iterations` limit is sufficiently high), which means that among other things the feerates don't actually matter. It has a few other weirdnesses:
* The randomization of the search order in this PR is mostly irrelevant (but still included so that the interface of `Linearize` is stable after this PR).
* The `SimpleCandidateFinder` in the fuzz tests is actual
...

🚀 achow101 merged a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403)

💬 achow101 commented on pull request "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results":
(https://github.com/bitcoin/bitcoin/pull/30408#issuecomment-2245854529)
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b