💬 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

📝 maflcko opened a pull request: "net: Log accepted connection after m_nodes.push_back; Fix intermittent test issue"
(https://github.com/bitcoin/bitcoin/pull/30512)
Fix the two issues reported in https://github.com/bitcoin/bitcoin/pull/30468/files#r1688444784:

* Delay a debug log line for consistency.
* Fix an intermittent test issue.

They are completely separate fixes, but both `net` related.

💬 maflcko commented on pull request "test: bump mocktime only after node has received and sent bytes":
(https://github.com/bitcoin/bitcoin/pull/30468#discussion_r1688462553)
Fixed in https://github.com/bitcoin/bitcoin/pull/30512

💬 maflcko commented on pull request "net: Log accepted connection after m_nodes.push_back; Fix intermittent test issue":
(https://github.com/bitcoin/bitcoin/pull/30512#issuecomment-2245863400)
Can be tested by adding a sleep before the LOCK:

```diff
diff --git a/src/net.cpp b/src/net.cpp
index ad43b316b3..a60aa5f31c 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -1813,2 +1813,3 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
{
+ UninterruptibleSleep(100ms);
LOCK(m_nodes_mutex);

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688468495)
It's similar, but not actually the same, because it isn't testing the exact result of `LinearizationChunking::Intersect`, but testing the *properties* it has we care about for LIMO/Merging to work (feerate of output is as high as feerate of input, and no intersection with a chunk prefix is better. Specifically, if you replace `LinearizationChunking::Intersect` with something that returns the highest-feerate intersection (rather than the first intersection) which has as high feerate as the input,
...

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688471698)
It should work, but wouldn't be testing the exact same thing (because the remaining part of a `LinearizationChunking` isn't required to be topological, and in practice it'll generally be the opposite: the full cluster with a topological subset removed).

🚀 achow101 merged a pull request: "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results"
(https://github.com/bitcoin/bitcoin/pull/30408)

💬 morehouse commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#discussion_r1688272087)
If time isn't attacker controlled, how would it go backwards? And how realistic is the range [-10m, +24h]? Naively, 24h seems like a long time between messages in the handshake.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688479676)
Will do if I retouch.

🤔 mzumsande reviewed a pull request: "index: Fix coinstats overflow and introduce index versioning"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-2194705197)
Concept ACK

Haven't tested anything yet, but did you check what happens in case of a downgrade? (an index generated with this PR is loaded with master)

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2245938009)
> Is the idea to incorporate the interface changes proposed in #30440 and #30409 after they're merged? And in the mean time keep them in #30437?

I could add them here. Just let me know what you prefer. I am planning to rebase #30437 on top of this, too.

> Maybe also link to #30437 in the description as it's a more simple demo than [Sjors#48](https://github.com/Sjors/bitcoin/pull/48).

Thanks, added

🚀 ryanofsky merged a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436)