feat: Compiler-Enforced Logging of All Miner Transaction Outcomes #2821
Conversation
src/core/mempool.rs
Outdated
| where | ||
| F: FnMut(Vec<MemPoolTxInfo>) -> Result<(), E>, | ||
| F: FnMut(MemPoolTxInfo) -> TransactionResult, |
There was a problem hiding this comment.
Note: iterate_candidate requires one TransactionResult at a time.
gregorycoppola
changed the title
src/chainstate/stacks/miner.rs
Outdated
| mined_origin_nonces: &mut HashMap<StacksAddress, u64>, | ||
| mined_sponsor_nonces: &mut HashMap<StacksAddress, u64>, | ||
| invalidated_txs: &mut Vec<Txid>, | ||
| block_limit_hit: &mut BlockLimitFunction, |
There was a problem hiding this comment.
The fact that you have to pass so many &mut references to this method from the caller's scope suggests to me that this is not the right way factor this.
src/chainstate/stacks/miner.rs
Outdated
| let transaction_result = StacksChainState::process_transaction(clarity_tx, tx, quiet); | ||
| match transaction_result { | ||
| TransactionResult::Success(TransactionSuccess { tx, fee, receipt }) => { | ||
| info!("Include tx"; |
There was a problem hiding this comment.
Why isn't this logging code in the queryable_logging.rs file? Same for the other long-winded structured logging calls below.
src/chainstate/stacks/miner.rs
Outdated
| @@ -292,21 +304,37 @@ impl<'a> StacksMicroblockBuilder<'a> { | |||
| } | |||
|
|
|||
| /// Mine the next transaction into a microblock. | |||
| /// Returns true/false if the transaction was/was not mined into this microblock. | |||
| /// | |||
| /// This is a wrapper around `StacksChainState::process_transaction` that also checks certain | |||
There was a problem hiding this comment.
"Wrapper" is not really the right word here. The mine_next_transaction method creates a block by processing a sequence of transactions it pulls from the mempool. The inner call to StacksChainState::process_transaction is used to faithfully create the MARF trie for this block and calculate the MARF state root for the block header. This is not the same as being a wrapper, since mine_next_transaction is not meant to be used to process blocks.
There was a problem hiding this comment.
There are a couple of problems with this PR, but they're easily fixed.
First, it doesn't seem to actually be logging anything in a queryable way, beyond just grepping the log file. It does seem to send the structured transaction-mining results to the event observer either. Do you intend to add either of these things? If not -- if the point of this PR is just to change what the miner prints out -- then a lot of this PR can be dropped and consolidated into src/chainstate/stacks/miner.rs as simply a set of structured types to represent each of the miner's decisions for a transaction.
Second, all of the transaction outcomes we're interested in are the concern of the miner, not the block database. The files in src/chainstate/stacks/db should not be touched by this PR at all, since they are not related to deciding which transactions to include and which to skip. The miner, which uses the block database to produce blocks, should instead interpret the result of processing each transaction it considers for mining on its own before deciding how to record the outcome of considering it.
|
Closed for #2975 |
Description
The goal of this PR is to provide more robust logging for the miner, so that:
To do this, we use the type system.
MemPoolDB::iterate_candidateswill now require to be given a new kind of object calledTransactionResult.There are three kinds of
TransactionResult:TransactionSuccess,TransactionErrorandTransactionSkipped.In order to create one of these types, the user should use a factory function, which will do the logging as a side-effect.
The logs look like this:
Also, the different kinds of events are collected, and can be used for further logging later.
Testing
I am running this in a local mock miner and observing the logs. We will also run this on Hiro's mock miner.
It doesn't really admit of unit tests right now, I think.
Limitations
It is currently still possible to create a
TransactionResultwithout using the factory methods, and that would mean that nothing gets logged. I'm not sure if it's technically possible to force the user to use the factory methods the first time an object is created, while also allowing them to decompose and reassemble objects inmatchblocks in Rust.