manojgop / sawtooth-poet Goto Github PK

Currently enclave can issue duration/wait certificate for a given block number multiple times when requested by POET engine running in untrusted code. Enclave cannot decide to issue duration/wait certificate just based on the most recent block number it stores in its context because it's possible during fork resolution that the current chain head is changed and POET engine needs to abandon current block and start building a new block which will have same block number as previous one. So one option is for the enclave to check for previous block id along with current block number before issuing duration/wait certificate. But even this approach is not foolproof unless the enclave maintains entire history of duration/wait certificate requests in its context.

Change the if condition here to == instead of !=. Allow out of date but after printing a error log trace.

Add IAS proxy in docker compose files, let client talk to IAS proxy and then go to IAS. With current behavior IAS Client talks to IAS directly.

hyper doesn't have timeout option for its requests. It needs to be handled externally, ideally the thread which runs should timeout waiting for ResponseFuture.

Main is too clutterred ----
Submodules to be brought in - check_consensus,fork resolver
Review moving extern crate declaration into specific files

@49 unnecessary - {}
@100 Rename requests
@113,114 What do these lines do??
@125 How to use stop_handle
@129 - Log the error as on 121

Engine ---

@19,20 remove if not needed
except() instead of unwrap()
clone() unwrap() - Replace wherever possible

@79 - LM not being used as of now
@79 - Avoid clone - in service as well for payload
@89 service.rs
@86 - Pass head here

See if BlockInvalid can be handled

@105,106 Remove comments
See the scope for avoiding validating one's own block in core

Does the validator push a future block to the consesnsus? ie - @5, block with block num 7 arrives

Figure out the difference in fail/ignore in sdk service call

@204 Comments here.. throughout as well
@203 Remove cert init code...

Consider removing handling of PeerMessage
@163 check what send_block_ack() does
log levels could be debug

@109 - Where to initialize after this/fail block??

The line pointed here does expect no error status from validator upon calling initialize_block(). But validator may return error if it fails to perform initialization and it will make PoET crash. It can be gracefully handled for different error returns from validator.

Currently it stores list of string and deserializes it when required, for catching errors early and code readability, we can store list of ValidatorRegistryMapEntry data

ValidatorRegistrySignupInfo parameter is read as string when ValidatroRegistryPayload is deserialized, it is later deserialized in getter method. These two steps can be combined, we will know if payload is not rightly composed at same step than distributing this knowledge at multiple places.

IasClient talks to IAS or IAS proxy, we do not know behavior of IAS when empty values are passed with keys (PSE manifest and nonce). If this is acceptable by IAS replace the hashmap in POST request in ias_client to struct.

Note: There's already a struct in ias_proxy, but since it only uses it to deserialize request we do not need hashmap in ias_proxy. IAS proxy doesn't construct the request, it just forwards whatever is received to IAS.

Reason for deserializing request in IAS proxy is to get quote from it, which is used to store in LRU cache.

Reason is same code is written at multiple places and is potential part of code for a utility function.

@29 - Rename init_wall_clock to service_start_time
@75 - Do away with remove in get_block. Prefer get then clone.
@90/@101 - Instead of while loop( attempt cancel or just continue) in summarize/finalize
@145 - Log and continue in cancel_block - Why panic necessarily if issue not at the end of consensus
@150 & @157 & @169- Remove
@186 - Return a tuple
@200 - Rename tagd to something more legible
@226 - Try using get instead of remove
@262 - Return ownership instead of clone()
@260 - See if hardcoded '#' can be abstracted
@430 - Remove this dummy test

enclave_sgx.rs has code related to enclave along with other things. It should ideally have only things related to enclave, as a wrapper. Move other functionalities from this to respective independent files.

Refactor the fork resolution module by making it more modularized.

1. Implement find_common_ancestor( function which takes the current chain head and the block id as argument and returns the common ancestor (i.e intersection of two linked lists).
2. Fork resolution module can return the status of fork resolution to engine.
3. Move the block consensus state change logic to engine. Engine can take actions like continue building on top of current chain head, discard the current block, update chain head to move to new fork etc.
4. Move the logic to update the consensus state store to engine and the functions which actually updates the state store to consensus state store module.