This comment is for a possible follow-up PR, not suggesting any change here, just wanted to document this thought.
As an alternative to this flag, it might be more natural to use “chained methods” so you could write something like:
0std::map<COutPoint, CAmount> node::MiniMiner(pool, outpoints)
1 .BuildMockTemplate(target_feerate)
2 .CalculateBumpFees();
(This technique is used here for example, and is common in more modern languages like Rust).
BuildMockTemplate() would return *this instead of void.
Of course, CalculateBumpFees() and CalculateTotalBumpFees() would not call BuildMockTemplate() as they currently do. We’d also need BuildMockTemplate() to save the feerate in the object for either of the calculate functions to us; we don’t want the caller to have to specify the same value twice. The feerate should probably be accessible using a getter.
A nice property of this approach is that for testing, the results of each “stage” could be instantiated into variables, and inspection methods or SanityCheck() could be called on them.
It should then be possible to make the two calculate methods const, which is intuitive – calculating from an object shouldn’t need to mutate it.
One other more subtle thing we’d want to do is handle the case where BuildMockTemplate() is called more than once – it would be nice to allow that even if initially only for testing. That is, the caller might do something like:
0node::MiniMiner miner(pool, outpoints);
1miner.BuildMockTemplate(feerate1);
2miner.BuildMockTemplate(feerate2);
3results = miner.CalculateBumpFees();
The effect you’d expect is for the bump fees to be based on feerate2. But the first call to BuildMockTemplate() would modify the object in such a way that, if nothing special is done, would mess up the second call to BuildMockTemplate().
I think the best way to deal with this is that when BuildMockTemplate() begins, it checks if there’s already a feerate set in the object (maybe m_feerate is type std::optional<CFeeRate>). If so, it resets of the object’s state to as it was after the constructor ran. (It shouldn’t need to actually re-run the constructor code; that may be a possible design but seems less elegant.)
One last thing, what would this do? (Note, BuildMockTemplate() isn’t called)
0results = node::MiniMiner(pool, outpoints).CalculateBumpFees();
I think what would be natural here is that all the bump fees would be zero, as if the target feerate is zero. In other words, the “default” target feerate is zero; BuildMockTemplate() simply modifies the feerate from zero.
Zero bump fees is also what can happen if we hit the cluster-too-large limit. We wouldn’t bump any fees, but that’s no worse than today. (I think this is what happens with the PR?)
