Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mining Rule Engine #634

Closed
wants to merge 15 commits into from
Closed

Conversation

coderofstuff
Copy link
Collaborator

@coderofstuff coderofstuff commented Feb 12, 2025

Mining Rule Engine

This is the full implementation the of the mining rule engine discussed in https://research.kas.pa/t/sync-monitor-mining-rule-engine/294

Concepts

  • Sync Rate - observed_number_of_blocks_received_per_interval / expected_number_of_blocks_to_receive_per_interval
  • Mining Rule - some rule that affect how and when a node should mine based on the conditions of the node

Changes

Outline

  1. Create a new Mining Rule Engine that currently tracks the sync rate of the node
  2. Move all is_nearly_synced logic to the rule engine area
  3. Introduce "Mining Rules" which are checked every 10s and depending on the conditions of the node triggers rules to be applied.

Mining Rule Engine

1. Implements a BlueOnlyParents strategy

  • Trigger: virtual processing time average is above 500ms
  • Recovery:
    • virtual processing time is back below 100ms AND
    • a merge bound amount of blocks has passed since the trigger

2. Implements a NoTransactions (empty blocks) strategy

  • Trigger: BadMerkleRoot count > valid block count in the interval
  • Recovery: 2 intervals have passed since the rule was triggered

3. Implements a SyncRateRule strategy

This is the implementation of the Sync Monitor discussed in the thread and doc above.

  1. Create a new Mining Rule Engine that currently tracks the sync rate of the node
  2. Introduce the Sync Rate Rule - allow mining if the sync rate of the node is lower than the sync rate threshold and the finality point is recent, even if the node is not "nearly synced"
  3. Move all is_nearly_synced logic to the rule engine area

- monitor sync state and update anything that cares about is_sycned to
  use the updated rules
- sync rate rule: allow mining if sync rate is below threshold and
  finality point is recent

Move most is_nearly_synced logic to mining_rule_engine

- instantiate Hub in daemon
- Reverse the dependency of flow context and MRE
- remove the async_is_nearly_synced consensus api
- add get_sink_daa_score_timestamp consensus API

Move is_nearly_synced logic from params to rule engine
- Overrides TX selection to instead automatically return an empty vec
- Includes only the logic that uses the rule
  - Make all reds "bad reds" when selected parents
  - Select 100% top blue work parents
- Trigger logic not present yet
- Add BlueParentsOnly Rule
- Add stub for NoTransactionsRule
- Makes it look like any other mining rule
@michaelsutton
Copy link
Contributor

virtual processing time does not cover things such as calc_block_parents which are only called as part of build_block_template_from_virtual_state. Perhaps that method should be added to the same timing counter.

In general I also think that virtual processing time needs to be normalized by the amount of UTXO-validated blocks during the resolve. This can be solved by changing self.counters.virtual_resolve_counts.fetch_add(1, Ordering::Relaxed) to fetch add such a count (and renaming the field). See chain_block_counts for a reference of such counting.

Other option (which I prefer): ignore UTXO validation and time only the parts of resolve virtual which prep for block template building: from before pick_virtual_parents and until after calculate_and_commit_virtual_state. This segment includes all sensitive header composing areas and, added with the time of build_block_template_from_virtual_state, should suffice for our cause.

One more comment re blue parents only rule: once triggered, this rule remains for a long period. So it seems wise to activate it only after a few consecutive slow rounds and not only after 10 seconds (or extend the monitor interval to 30 secs?)

@michaelsutton
Copy link
Contributor

The general structuring of the rules and the rule engine in terms of code design and style is top-notch btw

// enter the DAA window of fully-synced nodes and thus contribute to overall network difficulty
//
// [Crescendo]: both durations are nearly equal so this decision is negligible
unix_now()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duration is too long for 10BPS (~44 minutes = 26,400 block depth). To me, part of the motivation for identifying slow sync rate was to allow the definition of nearly synced to be tightened (as in testnets below). It's safer now bcs the network will still mine on a slowdown due to the new rule even if nearly synced = false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set it to as tight a bound as the one we have for testnet below? So, change this fn to behave the same way in all networks?

If it used to be 44 minutes at 1bps, something like 4.4mins at 10bps is an overwhelming amount of time for blocks not to pass. Perhaps even tighter?

If we consider 1min as the nearly synced bound, what are the consequences of that?

@@ -798,25 +817,32 @@ impl VirtualStateProcessor {
let mut ghostdag_data = self.ghostdag_manager.ghostdag(&virtual_parents);
let merge_depth_root = self.depth_manager.calc_merge_depth_root(&ghostdag_data, current_pruning_point);
let mut kosherizing_blues: Option<Vec<Hash>> = None;
let mut bad_reds = Vec::new();
let bad_reds = if self.mining_rules.blue_parents_only.load(Ordering::Relaxed) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, but I think it'll be nicer to have some wrapper methods for MiningRules around the atomic operations (and then we should probably make the members private)

/// Attempt to recover from high virtual processing times possibly caused merging red blocks.
/// by only pointing to blue parents.
///
/// Trigger: virtual processing average time is above threshold
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think slowness in build_block_template should trigger it. @hashdag and I also suggested slowness in header validation to trigger it

Trigger condition:
- if submit_block_bad_merkle_root_count > 0
  && submit_block_success_count == 0 (no submit block calls rpcs
  succeeded in this 10s period)

Recovery conditions:
- if submit_block_success_count > 0 (any submit block call succeeded,
  even if there are any that errored out), OR
- two cooldown periods have passed
@coderofstuff
Copy link
Collaborator Author

Closing this as it has been split in #654 #655 #656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants