💬 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
...
(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
...
(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
(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
(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.
(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.
(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.
(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
...
(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
...
(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
(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
...
(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
...
(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)
(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
(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.
(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
(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);
(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,
...
(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).
(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)
(https://github.com/bitcoin/bitcoin/pull/30408)