feat: remove reconcile mempool & debounce stats

[bestmike007]

Description

Currently when the event server handles a new_mempool_tx event, it reconcile mempool status and recalculate mempool stats. This is really unnecessary, especially when an influx of new transactions occurs. As the stacks node awaits the event server processing, it slows down the new block processing.

This PR suggests a method to reconcile and update mempool statistics through a debounce scheme. It significantly decreases the processing time for new_mempool_tx events from over 1s to less than 10ms.

Type of Change

Performance optimization.

Does this introduce a breaking change?

No.

Are documentation updates required?

No.

Testing information

Since the reconcile process is deferred, a method to wait for next reconcile is added for testing purposes. And the tests are updated accordingly.

Checklist

• Code is commented where needed
• Unit test coverage for new or modified code paths
• npm run test passes
• Changelog is updated
• Tag 1 of @rafaelcr or @zone117x for review

[bestmike007]

@rafaelcr @zone117x please help me review this. I'm not sure how to update the changelog.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (5ae0e96) 71.58% compared to head (d2b46f8) 71.62%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/datastore/pg-write-store.ts	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1815      +/-   ##
==========================================
+ Coverage   71.58%   71.62%   +0.04%     
==========================================
  Files          91       91              
  Lines       11750    11771      +21     
  Branches     2605     2612       +7     
==========================================
+ Hits         8411     8431      +20     
- Misses       3186     3187       +1     
  Partials      153      153              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zone117x]

Thanks for the PR! This is an interesting approach, but it looks like it will cause data consistency issues. For example, here is the comment on the reconcileMempoolStatus function:

  // Find any transactions that are erroneously still marked as both `pending` in the mempool table
  // and also confirmed in the mined txs table. Mark these as pruned in the mempool and log warning.
  // This must be called _after_ any writes to txs/mempool tables during block and microblock ingestion,
  // but _before_ any reads or view refreshes that depend on the mempool table.
  // NOTE: this is essentially a work-around for whatever bug is causing the underlying problem.

This PR moves the mempool tx status updating to outside of the sql transaction for /new_block ingestion. Which means clients will have period of time where a tx can appear to exist as both mined and still in the mempool.

As the above comment mentions, this reconcile function is a work-around for a bug that causes this inconsistent state. If we are to make changes around this, it would be better to track down and fix that bug, then we can remove the reconcileMempoolStatus function all together.

Fixing that bug involves looking through all the queries that perform an UPDATE/INSERT on the mempool_txs and txs tables. My guess is that the bug is around code for re-org handling.

[bestmike007]

I see, that will require more efforts. Does the mempool stats need to be consistent as well?

Is the bug cause by: the new mempool transaction event occurs after, not before, the new block event which settles the corresponding transaction?

[zone117x]

Does the mempool stats need to be consistent as well?

Not sure, I don't think that is as critical. The function was originally put in to address bug reports about inconsistent tx states.

Is the bug cause by: the new mempool transaction event occurs after, not before, the new block event which settles the corresponding transaction?

Possibly, not sure. If I knew, I'd fix it :)

Please feel free to take a look over the table update queries, it would be great to get that bug fixed!

[bestmike007]

@zone117x I updated the approach, please review again.

[bestmike007]

This avoid inconsistent states caused by new inserts.

Is the bug cause by: the new mempool transaction event occurs after, not before, the new block event which settles the corresponding transaction?

This is possible if the stacks-node does not check tx state before emitting new_mempool_transaction event. Theoretically, it SHOULD broadcast already settled transactions: they might be reorged and would be absent from the mempool table.

[bestmike007]

Doesn't look like the received transactions are filtered before forwarding to event observer:

https://github.com/stacks-network/stacks-core/blob/2.4.0.0.4/testnet/stacks-node/src/neon_node.rs#L2267-L2294
https://github.com/stacks-network/stacks-core/blob/2.4.0.0.4/src/net/relay.rs#L1815-L1857
https://github.com/stacks-network/stacks-core/blob/2.4.0.0.4/src/net/relay.rs#L1567-L1596

[rafaelcr]

LGTM, thanks for the contribution @bestmike007 💯

[blockstack-devops]

🎉 This PR is included in version 7.7.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀