Skip to content

Fix OOM error in large parentchain syncs with sidechain feature. #1493

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.

Sign up for GitHub

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

Merged
brenzi merged 15 commits into from
Nov 30, 2023
Merged

Fix OOM error in large parentchain syncs with sidechain feature. #1493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
986d2d5
improve log levels
clangenb Oct 14, 2023
d84bcd1
[service] `sidechain_start_untrusted_rpc_server`downgrade tokyo handl…
clangenb Nov 15, 2023
24522cd
[service/main] distinguish better between different setups
clangenb Nov 15, 2023
c2fac9f
[service/main] extract duplicate code
clangenb Nov 15, 2023
bc77a5b
[service/main] fix clippy
clangenb Nov 15, 2023
ee4bf37
[service/main] fix clippy with teeracle
clangenb Nov 15, 2023
9c6abe8
Merge branch 'master' into cl/fix-sidechain-oom
clangenb Nov 29, 2023
3acdc2e
introduce `is_syncing` in `sync_parentchain`
clangenb Nov 29, 2023
1ab60ee
remove obsolete `trigger_parentchain_block` ffi
clangenb Nov 29, 2023
fb3db5e
remove unused import
clangenb Nov 29, 2023
9c25d88
fix clippy
clangenb Nov 29, 2023
d907646
fix test compilation
clangenb Nov 29, 2023
4d65577
Merge branch 'master' into cl/fix-sidechain-oom
clangenb Nov 30, 2023
d49ec56
[service/main] extract `init_proxied_shard_vault`
clangenb Nov 30, 2023
54f378e
Merge branch 'master' into cl/fix-sidechain-oom
clangenb Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions core-primitives/enclave-api/ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,6 @@ extern "C" {
parentchain_id_size: u32,
) -> sgx_status_t;

pub fn trigger_parentchain_block_import(
eid: sgx_enclave_id_t,
retval: *mut sgx_status_t,
parentchain_id: *const u8,
parentchain_id_size: u32,
) -> sgx_status_t;

pub fn execute_trusted_calls(eid: sgx_enclave_id_t, retval: *mut sgx_status_t) -> sgx_status_t;

pub fn sync_parentchain(
Expand All @@ -86,6 +79,7 @@ extern "C" {
events_proofs_size: usize,
parentchain_id: *const u8,
parentchain_id_size: u32,
is_syncing: c_int,
) -> sgx_status_t;

pub fn set_nonce(
Expand Down
26 changes: 0 additions & 26 deletions core-primitives/enclave-api/src/enclave_base.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ pub trait EnclaveBase: Send + Sync + 'static {
parentchain_id: &ParentchainId,
) -> EnclaveResult<()>;

/// Trigger the import of parentchain block explicitly. Used when initializing a light-client
/// with a triggered import dispatcher.
fn trigger_parentchain_block_import(&self, parentchain_id: &ParentchainId)
-> EnclaveResult<()>;

fn set_nonce(&self, nonce: u32, parentchain_id: ParentchainId) -> EnclaveResult<()>;

fn set_node_metadata(
Expand Down Expand Up @@ -212,27 +207,6 @@ mod impl_ffi {

Ok(())
}
fn trigger_parentchain_block_import(
&self,
parentchain_id: &ParentchainId,
) -> EnclaveResult<()> {
let mut retval = sgx_status_t::SGX_SUCCESS;
let parentchain_id_enc = parentchain_id.encode();

let result = unsafe {
ffi::trigger_parentchain_block_import(
self.eid,
&mut retval,
parentchain_id_enc.as_ptr(),
parentchain_id_enc.len() as u32,
)
};

ensure!(result == sgx_status_t::SGX_SUCCESS, Error::Sgx(result));
ensure!(retval == sgx_status_t::SGX_SUCCESS, Error::Sgx(retval));

Ok(())
}

fn set_nonce(&self, nonce: u32, parentchain_id: ParentchainId) -> EnclaveResult<()> {
let mut retval = sgx_status_t::SGX_SUCCESS;
Expand Down
3 changes: 3 additions & 0 deletions core-primitives/enclave-api/src/sidechain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ pub trait Sidechain: Send + Sync + 'static {
events: &[Vec<u8>],
events_proofs: &[StorageProof],
parentchain_id: &ParentchainId,
is_syncing: bool,
) -> EnclaveResult<()>;

fn execute_trusted_calls(&self) -> EnclaveResult<()>;
Expand All @@ -56,6 +57,7 @@ mod impl_ffi {
events: &[Vec<u8>],
events_proofs: &[StorageProof],
parentchain_id: &ParentchainId,
is_syncing: bool,
) -> EnclaveResult<()> {
let mut retval = sgx_status_t::SGX_SUCCESS;
let blocks_enc = blocks.encode();
Expand All @@ -75,6 +77,7 @@ mod impl_ffi {
events_proofs_enc.len(),
parentchain_id_enc.as_ptr(),
parentchain_id_enc.len() as u32,
is_syncing.into(),
)
};

Expand Down
11 changes: 9 additions & 2 deletions core/parentchain/block-import-dispatcher/src/immediate_dispatcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,14 @@ impl<BlockImporter, SignedBlockType> DispatchBlockImport<SignedBlockType>
where
BlockImporter: ImportParentchainBlocks<SignedBlockType = SignedBlockType>,
{
fn dispatch_import(&self, blocks: Vec<SignedBlockType>, events: Vec<Vec<u8>>) -> Result<()> {
fn dispatch_import(
&self,
blocks: Vec<SignedBlockType>,
events: Vec<Vec<u8>>,
_is_syncing: bool,
) -> Result<()> {
// _is_syncing does not matter for the immediate dispatcher, behavoiur is the same. Immediate block import.

debug!("Importing {} parentchain blocks", blocks.len());
self.block_importer.import_parentchain_blocks(blocks, events)?;
debug!("Notifying {} observers of import", self.import_event_observers.len());
Expand Down Expand Up @@ -93,7 +100,7 @@ mod tests {
counter_clone.increment();
});

dispatcher.dispatch_import(vec![1u32, 2u32], vec![]).unwrap();
dispatcher.dispatch_import(vec![1u32, 2u32], vec![], false).unwrap();

assert_eq!(1, notification_counter.get_counter());
}
Expand Down
18 changes: 14 additions & 4 deletions core/parentchain/block-import-dispatcher/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,12 @@ pub trait DispatchBlockImport<SignedBlockType> {
/// Dispatch blocks to be imported.
///
/// The blocks may be imported immediately, get queued, delayed or grouped.
fn dispatch_import(&self, blocks: Vec<SignedBlockType>, events: Vec<Vec<u8>>) -> Result<()>;
fn dispatch_import(
&self,
blocks: Vec<SignedBlockType>,
events: Vec<Vec<u8>>,
is_syncing: bool,
) -> Result<()>;
}

/// Wrapper for the actual dispatchers. Allows to define one global type for
Expand Down Expand Up @@ -96,15 +101,20 @@ where
TriggeredDispatcher: DispatchBlockImport<SignedBlockType>,
ImmediateDispatcher: DispatchBlockImport<SignedBlockType>,
{
fn dispatch_import(&self, blocks: Vec<SignedBlockType>, events: Vec<Vec<u8>>) -> Result<()> {
fn dispatch_import(
&self,
blocks: Vec<SignedBlockType>,
events: Vec<Vec<u8>>,
is_syncing: bool,
) -> Result<()> {
match self {
BlockImportDispatcher::TriggeredDispatcher(dispatcher) => {
log::trace!("TRIGGERED DISPATCHER MATCH");
dispatcher.dispatch_import(blocks, events)
dispatcher.dispatch_import(blocks, events, is_syncing)
},
BlockImportDispatcher::ImmediateDispatcher(dispatcher) => {
log::trace!("IMMEDIATE DISPATCHER MATCH");
dispatcher.dispatch_import(blocks, events)
dispatcher.dispatch_import(blocks, events, is_syncing)
},
BlockImportDispatcher::EmptyDispatcher => {
log::trace!("EMPTY DISPATCHER DISPATCHER MATCH");
Expand Down
52 changes: 40 additions & 12 deletions core/parentchain/block-import-dispatcher/src/triggered_dispatcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,28 @@ where
&self,
blocks: Vec<SignedBlockType>,
events: Vec<RawEventsPerBlock>,
is_syncing: bool,
) -> Result<()> {
let parentchain_id = self.block_importer.parentchain_id();
trace!(
"[{:?}] Pushing parentchain block(s) and event(s) ({}) ({}) to import queue",
"[{:?}] Triggered dispatcher received block(s) and event(s) ({}) ({})",
parentchain_id,
blocks.len(),
events.len()
);
// Push all the blocks to be dispatched into the queue.
self.events_queue.push_multiple(events).map_err(Error::ImportQueue)?;
self.import_queue.push_multiple(blocks).map_err(Error::ImportQueue)
if is_syncing {
trace!(
"[{:?}] Triggered is in sync mode, immediately importing blocks and events",
parentchain_id
);
self.block_importer
.import_parentchain_blocks(blocks, events)
Copy link
Collaborator

@brenzi brenzi Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this simple is_syncing bool flag is not enough

  • It needs to be clear to other validateers what parentchain block is the first one which must be considered for indirect invocation (in SCV and OCW mode). therefore, that "first relevant parentchain block should be stored in the STF state. Most likely, we should define this as the block when the primary worker enclave got registered for the first time, because the shard state will only be written to once the primary worker is in sync and starts to process parentchain blocks. caveat: if the shardid differs from mrenclave and the shardid gets registered a lot later, we will be importing too many blocks - but I don't think this is critical
  • callin it "is_syncing" is misleading. we're always syncing. I think it should be called "is_initial_sync" or similar
  • for the initial sync, the events and indirect invocations shouldn't even be searched for and they shouldn't be validated using proofs to accelerate the process

Copy link
Contributor Author

@clangenb clangenb Dec 2, 2023

Choose a reason for hiding this comment

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

I agree with all points. I think there was a reason why it has been implemented the way it is now, but I forgot, and I don't see a reason why this is currently like that.

  • Substrate introduced the term is_major_sync, which is true whenever a node is far behind the best block. This concept does not really apply in our case, as we will send the light-client db to an outdated worker. However, what I am trying to say is that we stay somewhat in the substrate-jargon if we use is_initial_sync.

Copy link
Collaborator

@brenzi brenzi Dec 2, 2023

Choose a reason for hiding this comment

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

wouldn't it be more descriptive to call the flag ignore_invocations?

Copy link
Contributor Author

@clangenb clangenb Dec 3, 2023

Choose a reason for hiding this comment

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

hmmm, if this is the only thing that this flag is going to be used for yes, but I am not 100% if it is. Regardless, as long as we keep it like that, I agree. It could also be something like: verify_only.

Copy link
Contributor Author

@clangenb clangenb Dec 3, 2023

Choose a reason for hiding this comment

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

Ah, we could also use this as some point maybe for non-authoring sidechain rpc-nodes, if we ever want to have them.

.map_err(Error::BlockImport)
} else {
trace!("[{:?}] pushing blocks and events to import queues", parentchain_id);
self.events_queue.push_multiple(events).map_err(Error::ImportQueue)?;
self.import_queue.push_multiple(blocks).map_err(Error::ImportQueue)
}
}
}

Expand Down Expand Up @@ -167,6 +178,7 @@ where
&self,
predicate: impl Fn(&BlockImporter::SignedBlockType) -> bool,
) -> Result<Option<BlockImporter::SignedBlockType>> {
trace!("Import of parentchain blocks and events has been triggered");
let blocks_to_import =
self.import_queue.pop_until(predicate).map_err(Error::ImportQueue)?;

Expand Down Expand Up @@ -232,7 +244,11 @@ mod tests {
let dispatcher = test_fixtures();

dispatcher
.dispatch_import(vec![1, 2, 3, 4, 5], vec![vec![1], vec![2], vec![3], vec![4], vec![5]])
.dispatch_import(
vec![1, 2, 3, 4, 5],
vec![vec![1], vec![2], vec![3], vec![4], vec![5]],
false,
)
.unwrap();

assert!(dispatcher.block_importer.get_all_imported_blocks().is_empty());
Expand All @@ -248,10 +264,14 @@ mod tests {
let dispatcher = test_fixtures();

dispatcher
.dispatch_import(vec![1, 2, 3, 4, 5], vec![vec![1], vec![2], vec![3], vec![4], vec![5]])
.dispatch_import(
vec![1, 2, 3, 4, 5],
vec![vec![1], vec![2], vec![3], vec![4], vec![5]],
false,
)
.unwrap();
dispatcher
.dispatch_import(vec![6, 7, 8], vec![vec![6], vec![7], vec![8]])
.dispatch_import(vec![6, 7, 8], vec![vec![6], vec![7], vec![8]], false)
.unwrap();

assert!(dispatcher.block_importer.get_all_imported_blocks().is_empty());
Expand All @@ -266,7 +286,7 @@ mod tests {
fn triggering_import_all_empties_queue() {
let dispatcher = test_fixtures();

dispatcher.dispatch_import(vec![1, 2, 3, 4, 5], vec![]).unwrap();
dispatcher.dispatch_import(vec![1, 2, 3, 4, 5], vec![], false).unwrap();
let latest_imported = dispatcher.import_all().unwrap().unwrap();

assert_eq!(latest_imported, 5);
Expand All @@ -278,7 +298,7 @@ mod tests {
fn triggering_import_all_on_empty_queue_imports_none() {
let dispatcher = test_fixtures();

dispatcher.dispatch_import(vec![], vec![]).unwrap();
dispatcher.dispatch_import(vec![], vec![], false).unwrap();
let maybe_latest_imported = dispatcher.import_all().unwrap();

assert!(maybe_latest_imported.is_none());
Expand All @@ -295,7 +315,11 @@ mod tests {
let dispatcher = test_fixtures();

dispatcher
.dispatch_import(vec![1, 2, 3, 4, 5], vec![vec![1], vec![2], vec![3], vec![4], vec![5]])
.dispatch_import(
vec![1, 2, 3, 4, 5],
vec![vec![1], vec![2], vec![3], vec![4], vec![5]],
false,
)
.unwrap();
let latest_imported =
dispatcher.import_until(|i: &SignedBlockType| i == &4).unwrap().unwrap();
Expand All @@ -311,7 +335,11 @@ mod tests {
let dispatcher = test_fixtures();

dispatcher
.dispatch_import(vec![1, 2, 3, 4, 5], vec![vec![1], vec![2], vec![3], vec![4], vec![5]])
.dispatch_import(
vec![1, 2, 3, 4, 5],
vec![vec![1], vec![2], vec![3], vec![4], vec![5]],
false,
)
.unwrap();
let maybe_latest_imported = dispatcher.import_until(|i: &SignedBlockType| i == &8).unwrap();

Expand All @@ -328,7 +356,7 @@ mod tests {
fn trigger_import_all_but_latest_works() {
let dispatcher = test_fixtures();

dispatcher.dispatch_import(vec![1, 2, 3, 4, 5], vec![]).unwrap();
dispatcher.dispatch_import(vec![1, 2, 3, 4, 5], vec![], false).unwrap();
dispatcher.import_all_but_latest().unwrap();

assert_eq!(dispatcher.block_importer.get_all_imported_blocks(), vec![1, 2, 3, 4]);
Expand Down
7 changes: 2 additions & 5 deletions enclave-runtime/Enclave.edl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,14 @@ enclave {
[in, size=parentchain_id_size] uint8_t* parentchain_id, uint32_t parentchain_id_size
);

public sgx_status_t trigger_parentchain_block_import(
[in, size=parentchain_id_size] uint8_t* parentchain_id, uint32_t parentchain_id_size
);

public sgx_status_t execute_trusted_calls();

public sgx_status_t sync_parentchain(
[in, size=blocks_size] uint8_t* blocks, size_t blocks_size,
[in, size=events_size] uint8_t* events, size_t events_size,
[in, size=events_proofs_size] uint8_t* events_proofs, size_t events_proofs_size,
[in, size=parentchain_id_size] uint8_t* parentchain_id, uint32_t parentchain_id_size
[in, size=parentchain_id_size] uint8_t* parentchain_id, uint32_t parentchain_id_size,
int is_syncing
);

public sgx_status_t set_nonce(
Expand Down
Loading