Feat/miner structured logging #2975
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2975 +/- ##
===========================================
+ Coverage 82.63% 82.65% +0.02%
===========================================
Files 237 237
Lines 193149 193607 +458
===========================================
+ Hits 159616 160035 +419
- Misses 33533 33572 +39
Continue to review full report at Codecov.
|
| @@ -445,23 +726,21 @@ impl<'a> StacksMicroblockBuilder<'a> { | |||
| self.runtime.num_mined = num_txs; | |||
|
|
|||
| match result { | |||
| Err(Error::BlockTooBigError) => { | |||
There was a problem hiding this comment.
I moved this code block to the ProcessingError branch of the match just above.
This piece of code was actually never executed before, because mine_next_transaction never returned an Err.
| submit_tx(&http_origin, &tx); // should succeed | ||
| submit_tx(&http_origin, &tx_2); // should fail since it tries to publish contract with same name | ||
| submit_tx(&http_origin, &mb_tx); // should be in microblock bc it is microblock only | ||
| sleep_ms(20_000); |
There was a problem hiding this comment.
is the sleep needed? is next_block_and_wait enough?
| // second block will be the first mined Stacks block | ||
| next_block_and_wait(&mut btc_regtest_controller, &blocks_processed); | ||
|
|
||
| let account = get_account(&http_origin, &miner_account); |
There was a problem hiding this comment.
we copy these "get_account" checks around but i don't think it's needed for the test
There was a problem hiding this comment.
True... but I guess it doesn't hurt to ensure that the starting state of all the relevant accounts are what you expect them to be? Let me know what you think.
There was a problem hiding this comment.
I personally think it's distracting to check things that aren't part of the test. There's an infinite number of things we could assert about the program at any given time.
We presumably have other tests that cover things that are not under test.
It obscures what is being tested to have extra asserts.
|
@kantai @jcnelson is there an "ontology" of different error subtypes in the transaction processor? How do we know which errors would cause a block to stop processing (ie Do we basically always continue if the block isn't full? Are we panicking on all unrecoverable errors so that any Err error is recoverable? |
|
If we're able to change in the production code "whether or not an error-like event aborts processing for the batch", and not break any tests, then it would probably add a lot of value to add some integration tests that would capture that. I.e., the test would break if we changed the error class of each error event. Probably that's a big thing to ask but eventually would be very useful. This is really a key component of the blockchain flow. |
|
Thanks again for doing a fast job on this, @pavitthrap . Part of the goal of making this PR was to discover an error ontology by trying this change and having it reviewed. So, hopefully doesn't seem like a drag to be iterating on the solution. We're clarifying the API a lot and also got the events out of it. |
There was a problem hiding this comment.
These changes look good to me, thanks @pavitthrap! Just had a few comments about the TransactionResult handling in the miner.
|
Thanks for the help finishing this, @kantai . |
…ns being processed
There was a problem hiding this comment.
Eagerly awaiting the landing of this PR.
I just had a few very small points and I am LGTM.
However, with so little test coverage in the area, I leave it up Aaron and Pavitthra's knowledge of the code paths and ability to read code carefully in order to ensure we don't break existing logic/consensus.
| // second block will be the first mined Stacks block | ||
| next_block_and_wait(&mut btc_regtest_controller, &blocks_processed); | ||
|
|
||
| let account = get_account(&http_origin, &miner_account); |
There was a problem hiding this comment.
I personally think it's distracting to check things that aren't part of the test. There's an infinite number of things we could assert about the program at any given time.
We presumably have other tests that cover things that are not under test.
It obscures what is being tested to have extra asserts.
Description
Re-implements #2821: This PR adds the type
TransactionResult, which is used to track outcomes of transaction inclusion by miners in both blocks and microblocks.There are three result types for a transactions:
Moreover, this PR updates the
mined_blocksevent and adds themined_microblocksevent. Each of these events has a fieldtx_events, which is a list ofTransactionEvents(which are constructed fromTransactionResults; they contain a subset of that data)Areas for Feedback
miner_observers_lookupto determine which observers to sendmined_blocksevents too. Instead of using this set, I created another set,mined_microblocks_observers_lookupfor the newmined_microblocksevent. I could:mined_microblocks_observers_lookupand just useminer_observers_lookupfor bothmined_blocksandmined_microblockseventsminer_observers_lookupto bemined_blocks_observers_lookupminer_observers_lookup,mined_blocks_observers_lookupandmined_microblocks_observers_lookup. The first groupminer_observers_lookupwould subscribe to both themined_blocksandmined_microblockseventsTransactionSuccessEventMinedMicroblockEventTesting
Added
mining_events_integration_testtoneon_integrations