🤔 ajtowns reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2721482897)
Looks good

💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2016650965)
I wonder if getting chunks in feerate order across multiple clusters might be something common/important enough to be worth doing more directly, rather than just getting them all then sorting them after the fact? I suppose I'd just be suggesting using a heap of affected clusters, which isn't that different to just sorting by chunk, so I guess it's fine.

💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2016595434)
Not sure if it's worth it, but I think you could return `vector<FeePerWeight>` if you also had:

```c++
template <typename T>
requires std::is_base_of_v<FeeFrac, T> && (sizeof(FeeFrac) == sizeof(T))
std::partial_ordering CompareChunks(std::span<const T> chunks0, std::span<const T> chunks1)
{
return CompareChunks(std::span<const FeeFrac>(reinterpret_cast<const FeeFrac*>(chunks0.data()), chunks0.size()), std::span<const FeeFrac>(reinterpret_cast<const FeeFrac*>(chunks1.data()), chunks1.
...

💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017036847)
Perhaps this stuff could be a little bit more RAII-ish? Something like:

```c++
class RefsVector : public std::vector<Ref*>
{
TxGraph& m_graph;
public:
template<typename... T>
RefsView(TxGraph& graph, T&&... args) : std::vector<Ref*>{args...}, m_graph{graph} { ++m_graph.m_chunkindex_observers; }
~RefsView() { --m_graph.m_chunkindex_observers; }
};
```

Might be good to introduce `m_chunkindex_observers` in its own commit? Arguably having a vector of Ref's (making it
...

💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017013375)
I'm a bit surprised to see `m_current_chunk` as a persistent-ish variable that includes `Ref*` -- that makes it a bug to move `Ref`s around while you have a `BlockBuilder` in existence. I would have thought something more like:

```c++
struct NextChunk { std::vector<Ref*> refs; FeePerWeight feerate; }
optional<NextChunk> GetNextChunk();
void Include(); void Skip();

...

while (auto x = bb.GetNextChunk(); x) {
if (x->feerate.size > remaining) { bb.Skip()
...

💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2016719978)
Would it make sense to have `, int level=MAIN_LEVEL` as a default argument so this could be used for (items changed in) staging as well? Otherwise, should the class name or docstring reflect it's for main only?

💬 ajtowns commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2017115908)
Should check `observers==0` in `UpdateRef` too?

💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2758766640)
_<ins>Updates</ins>:_
- Addressed @furszy's feedback.

💬 pinheadmz commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2758910021)
This link is incorrect:

> [4.1 Introduce new getdescriptoractivity rpc to track activity of a given set of descriptors](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide#41-introduce-new-getdescriptoractivity-rpc-to-track-activity-of-a-given-set-of-descriptors)([#28121](https://github.com/bitcoin/bitcoin/pull/28121))


it should be #30708

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2758924007)
Rebased 418236c106e32abd7357551d309f8e6d1e494f72 -> 155977ae8b4280100577455d34afa4d26ce89c58 ([`pr/wrap.25`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.25) -> [`pr/wrap.26`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.26), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.25-rebase..pr/wrap.26)) due to conflict with #32145

💬 instagibbs commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017245611)
should make it clear that `tx` must be in `todo`

💬 instagibbs commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017280508)
setr

💬 instagibbs commented on pull request "Follow-ups for txgraph #31363":
(https://github.com/bitcoin/bitcoin/pull/32151#discussion_r2017269249)
I modified it to not gate on existence in `todo`, and nothing seems to be breaking. Shouldn't this cause an `Assume` failure when holes existed in the serialization and the chosen number selects that unused hole?

💬 Prabhat1308 commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2758941205)
> This link is incorrect:
>
> > [4.1 Introduce new getdescriptoractivity rpc to track activity of a given set of descriptors](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide#41-introduce-new-getdescriptoractivity-rpc-to-track-activity-of-a-given-set-of-descriptors)([#28121](https://github.com/bitcoin/bitcoin/pull/28121))
>
> it should be [#30708](https://github.com/bitcoin/bitcoin/pull/30708)

Fixed

👍 ryanofsky approved a pull request: "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/31874#pullrequestreview-2722957738)
Code review ACK 35d17cd5eecabd36a2a14b35a47de22726ede0f8

👍 l0rinc approved a pull request: "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/31874#pullrequestreview-2722972767)
ACK 35d17cd5eecabd36a2a14b35a47de22726ede0f8

💬 glozow commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2759004140)
I suggest we reconsider this. To add on to the earlier points in favor of this approach, I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.

Additionally, if we want to mirror the Great Consensus Cleanup, it now disallows ==64 instead of <=64 (see https://github.com/bitcoin/bips/pull/1800). So the "We've previously had [a proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-March/01671
...

📝 glozow unlocked a pull request: "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only"
(https://github.com/bitcoin/bitcoin/pull/26398)
Since the original fix was set to be a "reasonable" transaction
to reduce allocations and the true motivation later revealed,
it makes sense to relax this check to something more principled.

There are more exotic transaction patterns that could take advantage
of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn
a utxo to fees for CPFP purposes when change isn't practical.

Two changes could be accomplished:

1) Anything not 64 bytes could be allowed

2) Anything abo
...

💬 instagibbs commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2759011537)
I can rebase this, or @darosior can if he likes. Let's just not duplicate work.

💬 darosior commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2759011631)
I agree. I'll rebase this and open a new PR unless @instagibbs beats me to it here.