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

📝 hebasto opened a pull request: "depends: Bump `libmultiprocess` for CMake fixes"
(https://github.com/bitcoin/bitcoin/pull/30513)
This PR amends https://github.com/bitcoin/bitcoin/pull/30490 and bumps the upstream branch, which now includes a required CMake [fix](https://github.com/chaincodelabs/libmultiprocess/pull/103).

Addresses https://github.com/bitcoin/bitcoin/pull/30490#issuecomment-2241153244.

The CI logs are available in https://github.com/bitcoin/bitcoin/pull/29790 where the recent [push](https://github.com/hebasto/bitcoin/tree/pr29790-0723.2.mp) uses this PR implementation.

💬 hebasto commented on pull request "depends: Bump `libmultiprocess` for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30513#issuecomment-2245989301)
cc @ryanofsky @theuni