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

💬 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".

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

💬 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

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

🤔 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:

...

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

💬 l0rinc commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2018767338)
Was the `const std::string_view` kept in the `.cpp` file on purpose?
I've noticed we usually avoid consting a view...

I see we even have `const std::string_view&` - would it make sense to unify those usages here as well?

💬 l0rinc commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2019211614)
A similar one may be
```patch
diff --git a/src/netbase.h b/src/netbase.h
--- a/src/netbase.h (revision 29a5a50583920e839fc3020c08f28b2f12a64d74)
+++ b/src/netbase.h (date 1743172625447)
@@ -60,7 +60,7 @@
public:
Proxy() : m_is_unix_socket(false), m_randomize_credentials(false) {}
explicit Proxy(const CService& _proxy, bool _randomize_credentials = false) : proxy(_proxy), m_is_unix_socket(false), m_randomize_credentials(_randomize_credentials) {}
- explicit Proxy(const std:
...

📝 olaristo109 opened a pull request: "Fix legacy migration bug"
(https://github.com/bitcoin/bitcoin/pull/32161)
Fix: Prevent crash on null legacy_spkm in GetDescriptorsForLegacy

This pull request resolves a critical issue that caused Bitcoin Core to crash during legacy wallet migration. The 'GetDescriptorsForLegacy' function was not properly handling cases where 'legacy_spkm' was a null pointer, leading to a dereference error.

This PR implements the following changes:

* Adds an explicit null pointer check for 'legacy_spkm'.
* Adds detailed error logging, including the key associated with the nul
...

💬 davidgumberg commented on pull request "Fix legacy migration bug":
(https://github.com/bitcoin/bitcoin/pull/32161#discussion_r2019321308)
Why?

💬 glozow commented on issue "RPC: `getblockstats` might not return the *effective* percentile fee rate":
(https://github.com/bitcoin/bitcoin/issues/31140#issuecomment-2762446017)
I hope this doesn't sound too dismissive, but who uses this and what are they using it for? Can we delete these fields or the whole RPC? To visualize stats, I think using `getblock` to obtain the fees and sizes for each transaction is much more flexible and appropriate (though tbf I don't think this was the case when #10757 was originally opened).

Separately, linearizing and chunking transactions (whether it's a block or something else) is pretty complicated and perhaps worth exposing it or off
...

🤔 TheCharlatan reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2726753659)
Guix builds:
```
aarch64
6ea4a76be3383337e57d6a12450bd589776ebb3fd0d9161347766ef845241e13 guix-build-c4861570e468/output/aarch64-linux-gnu/SHA256SUMS.part
3eb7656483dfe47fa6b7cf40bceb3decda73474c813edb224d42840adb8b49d6 guix-build-c4861570e468/output/aarch64-linux-gnu/bitcoin-c4861570e468-aarch64-linux-gnu-debug.tar.gz
0aa522010efd138d78eeac0a8ea15df469298c50afaa2451dece78c564546cac guix-build-c4861570e468/output/aarch64-linux-gnu/bitcoin-c4861570e468-aarch64-linux-gnu.tar.gz
5e1e833519
...

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019375703)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019376118)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019371904)
Added a comment.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019372075)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019372720)
No, it's being assigned to below.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019374546)
I've changed the code so that `Include()` and `Skip()` are no-ops once the end is reached.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019375780)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019374728)
Same here, made it have no effect in that case.