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

feat(engine)!: syncing published templates #1222

Draft
wants to merge 49 commits into
base: development
Choose a base branch
from

Conversation

ksrichard
Copy link
Collaborator

Description

Motivation and Context

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

ksrichard and others added 30 commits November 29, 2024 14:58
…h-template-as-substate

# Conflicts:
#	dan_layer/consensus/src/consensus_constants.rs
#	dan_layer/consensus_tests/src/support/harness.rs
#	dan_layer/engine/src/transaction/error.rs
#	dan_layer/engine/src/transaction/processor.rs
…h-template-as-substate

# Conflicts:
#	applications/tari_dan_app_utilities/src/base_layer_scanner.rs
#	dan_layer/engine_types/src/substate.rs
Copy link

github-actions bot commented Dec 19, 2024

Test Results (CI)

0 tests   - 593   0 ✅  - 593   0s ⏱️ - 3h 31m 53s
0 suites  -  66   0 💤 ±  0 
0 files    -   2   0 ❌ ±  0 

Results for commit afdb18b. ± Comparison against base commit dadfd5d.

♻️ This comment has been updated with latest results.

info!(target: LOG_TARGET, "Template manager initializing");
// Template manager
let (tx_template_sync, rx_template_sync) = broadcast::channel::<TemplateSyncRequest>(100);
Copy link
Member

Choose a reason for hiding this comment

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

use an unbounded mpsc channel. Broadcast channel is not appropriate as it is typically used for events and if there are more than 100 requests some of them may start to be dropped (broadcast channel lagging)

use tari_engine_types::substate::SubstateId;

/// General trait for different state stores to do operations on substates.
pub trait SubstateStore {
Copy link
Member

Choose a reason for hiding this comment

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

This doesnt seem right. Firstly, this is in the SQLite implementation of the state store which will be a problem when we switch to rocksDB. Templates can always be version 0, so not sure this is necessary, I'll need to look more closely at the rest of the code. There is currently no special process for upgrading the template code, you simply publish a new template at v0.

}
if let Some(TemplateResult::Template(fetched_template)) = self.fetch_template(address).optional()? {
template = Some(fetched_template);
}
Copy link
Member

Choose a reason for hiding this comment

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

break here?

template = Some(fetched_template);
}
// sleeping here to not overload the local database while waiting for the template to be ready
thread::sleep(std::time::Duration::from_millis(100));
Copy link
Member

Choose a reason for hiding this comment

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

Not ideal. Will think about how we should handle pending templates in a transaction.

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

Successfully merging this pull request may close these issues.

3 participants