💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018913351)
maybe overkill but `m_main_chunkindex` since there's a lot of level==0 checks everywhere gating access/modification
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018913351)
maybe overkill but `m_main_chunkindex` since there's a lot of level==0 checks everywhere gating access/modification
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018886603)
1d500a50b698a638c840ca79504c2a9a421e8756 stretch goal, have SanityCheck check m_chunk_count directly?
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018886603)
1d500a50b698a638c840ca79504c2a9a421e8756 stretch goal, have SanityCheck check m_chunk_count directly?
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018829062)
micro-nit: I know they're identical, but having the list being built via `stage_real_diagram` seems easier to digest what's being computed
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018829062)
micro-nit: I know they're identical, but having the list being built via `stage_real_diagram` seems easier to digest what's being computed
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018906562)
nit: Could we note that this is initialized first in `AddTransaction`?
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018906562)
nit: Could we note that this is initialized first in `AddTransaction`?
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019037611)
```Suggestion
// The same order should be obtained through a BlockBuilder as implied by CompareMainOrder, if nothing is skipped.
```
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019037611)
```Suggestion
// The same order should be obtained through a BlockBuilder as implied by CompareMainOrder, if nothing is skipped.
```
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018984013)
b130e185f54c7f762da6ef435882335e4f0b98ff
Could the documentation get beefed up a bit for this? What the shape that range should look like, etc. Was only clear later when seeing usages.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018984013)
b130e185f54c7f762da6ef435882335e4f0b98ff
Could the documentation get beefed up a bit for this? What the shape that range should look like, etc. Was only clear later when seeing usages.
💬 glozow commented on pull request "fuzz: extract unsequenced operations with side-effects":
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2762140874)
Backported in #32136
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2762140874)
Backported in #32136
💬 glozow commented on pull request "cmake: Add `NO_CACHE_IF_FAILED` option for checking linker flags":
(https://github.com/bitcoin/bitcoin/pull/32027#issuecomment-2762141394)
Backported in #32136
(https://github.com/bitcoin/bitcoin/pull/32027#issuecomment-2762141394)
Backported in #32136
💬 glozow commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2762141871)
Backported in #32136
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2762141871)
Backported in #32136
💬 yancyribbens commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2019167894)
> The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:
This is an annoying draw back that the stack trace won't contain the line in the test that's in error, but instead the line in the `assert_not_equal` helper. In Rust, one can decorate the helper function with the `track_caller` macro. I don't see anything that is similar for C++ sadly.
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2019167894)
> The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:
This is an annoying draw back that the stack trace won't contain the line in the test that's in error, but instead the line in the `assert_not_equal` helper. In Rust, one can decorate the helper function with the `track_caller` macro. I don't see anything that is similar for C++ sadly.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019170353)
As far as diagram is concerned (incl. for `FeeRateCompare`), the order of equal-feerate element is irrelevant, as they just represent different sections of an overall line segment with a fixed slope. As far as `FeeRateCompare` is concerned, they could even be merged into a single line segment with the combined fee and size.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019170353)
As far as diagram is concerned (incl. for `FeeRateCompare`), the order of equal-feerate element is irrelevant, as they just represent different sections of an overall line segment with a fixed slope. As far as `FeeRateCompare` is concerned, they could even be merged into a single line segment with the combined fee and size.
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019171810)
Because fees can be negative in the fuzz test. If they were all non-negative, then I think this suggested assertion should hold.
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019171810)
Because fees can be negative in the fuzz test. If they were all non-negative, then I think this suggested assertion should hold.
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019173335)
ah! please leave a note as to that, I always forget that's possible
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019173335)
ah! please leave a note as to that, I always forget that's possible
💬 Christewart commented on pull request "RFC: Accept non-std transactions in Testnet4 by default again":
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2762182397)
> > the question is why we are not also moving back on this setting, too, so that people can use non-std transactions on Testnet4.
>
> I don't think this is enough. There are many non-std transactions that will still be rejected, even if this is turned on. One example is #29843.
Another example is 64-byte transactions which was surprising behavior to me when working the BIP to disallow them:
https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/src/validation.
...
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2762182397)
> > the question is why we are not also moving back on this setting, too, so that people can use non-std transactions on Testnet4.
>
> I don't think this is enough. There are many non-std transactions that will still be rejected, even if this is turned on. One example is #29843.
Another example is 64-byte transactions which was surprising behavior to me when working the BIP to disallow them:
https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/src/validation.
...
💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019177841)
Oh, I misread this, but I'm still having trouble understanding what is intended. "Sort by feerate" ... "as these are just feerates".
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019177841)
Oh, I misread this, but I'm still having trouble understanding what is intended. "Sort by feerate" ... "as these are just feerates".
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019183993)
Maybe I'm making things more confusing by mentioning it at all?
My thinking is this: in general, to construct the full-graph chunk ordering, you *cannot* just sort by feerate, because you may sort two equal-feerate chunks within the same cluster incorrectly w.r.t. each other, breaking topology. However, here we are just construct a diagram, and don't care about what chunks they correspond to, so as far as the feerates of line segments in the diagram is concerned, we can just ignore this probl
...
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019183993)
Maybe I'm making things more confusing by mentioning it at all?
My thinking is this: in general, to construct the full-graph chunk ordering, you *cannot* just sort by feerate, because you may sort two equal-feerate chunks within the same cluster incorrectly w.r.t. each other, breaking topology. However, here we are just construct a diagram, and don't care about what chunks they correspond to, so as far as the feerates of line segments in the diagram is concerned, we can just ignore this probl
...
💬 olaristo109 commented on issue "wallet: migratewallet crashes "Assertion `legacy_spkm' failed"":
(https://github.com/bitcoin/bitcoin/issues/32112#issuecomment-2762195697)
Thanks for reporting this! I've fixed the 'GetDescriptorsForLegacy' crash by adding a null check and better error logging. I'm testing now
(https://github.com/bitcoin/bitcoin/issues/32112#issuecomment-2762195697)
Thanks for reporting this! I've fixed the 'GetDescriptorsForLegacy' crash by adding a null check and better error logging. I'm testing now
💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019188834)
Can you elaborate, or suggest code?
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019188834)
Can you elaborate, or suggest code?
🤔 l0rinc reviewed a pull request: "refactor: Enforce readability-avoid-const-params-in-decls"
(https://github.com/bitcoin/bitcoin/pull/31650#pullrequestreview-2725702425)
Concept ACK - this should clear up a few const usages.
After rebasing I found a few other similar ones, but not sure if they were ignored on purpose or not.
Please consider the ones that are related to this change - and just skip the ones that you've ignored on purpose.
<details>
<summary>Details</summary>
```patch
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index 8370179395..9dcceec739 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -471,7 +471,7 @@ public:
...
(https://github.com/bitcoin/bitcoin/pull/31650#pullrequestreview-2725702425)
Concept ACK - this should clear up a few const usages.
After rebasing I found a few other similar ones, but not sure if they were ignored on purpose or not.
Please consider the ones that are related to this change - and just skip the ones that you've ignored on purpose.
<details>
<summary>Details</summary>
```patch
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index 8370179395..9dcceec739 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -471,7 +471,7 @@ public:
...
💬 l0rinc commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2019210398)
argsman_tests.cpp
```C++
void ReadConfigString(const std::string& str_config)
void SetNetworkOnlyArg(const std::string& arg)
```
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2019210398)
argsman_tests.cpp
```C++
void ReadConfigString(const std::string& str_config)
void SetNetworkOnlyArg(const std::string& arg)
```