Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem: duplicate storage between chain-abci and tx-validation app (fixes #866) #1073

Merged
merged 1 commit into from
Feb 16, 2020

Conversation

tomtau
Copy link
Contributor

@tomtau tomtau commented Feb 14, 2020

Solution: as a part of ADR-001: https://github.com/crypto-com/chain/blob/master/architecture-docs/adr-001.md

  • tx-validation-app moved to chain-abci
  • chain-abci build process expanded to handle SGX SDK steps for C stubs -- on non-Linux systems, it'd display a warning and compile the mock version
  • tx-validation-app SGX unit test moved to chain-abci under a "sgx-test" feature flag
  • tx-query-app test removed (it was a kind of "mini-integration" / functionality test that assumed a lot of old behaviour, and all of this is now tested in integration tests)
  • enclave-bridge takes "intra enclave" requests that are passed directly to the ecalls and returns the response
  • ZMQ server started in chain-abci in a separate thread to handle tx-query requests (note: tx-query was out of scope of ADR-001, as it doesn't have any storage -- its future is TBD depending on audit feedback etc.)
  • redundant enclave protocol variants removed
  • "readonly" storage version provided for serving tx-query requests -- rocksdb/kvdb is thread-safe... zmq server then takes the latest chain state or sealed transactions directly -- note: some fixes related to fees, enclave protocol etc. (other steps of ADR-001) would be addressed in a separate PR
  • chain-abci storage expanded with one column for sealed transaction payloads
  • integration test building and environment preparation updated

note: Makefile + chain-docs aren't updated yet / would be addressed in separate PRs

@tomtau
Copy link
Contributor Author

tomtau commented Feb 14, 2020

bors try

bors bot added a commit that referenced this pull request Feb 14, 2020
@bors
Copy link
Contributor

bors bot commented Feb 14, 2020

try

Build failed

chain-abci/src/app/app_init.rs Outdated Show resolved Hide resolved
chain-abci/src/app/app_init.rs Outdated Show resolved Hide resolved
chain-abci/src/enclave_bridge/mod.rs Show resolved Hide resolved
chain-abci/tests/tx_validation.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@leejw51crypto leejw51crypto left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@yihuang yihuang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@calvinlauyh calvinlauyh left a comment

Choose a reason for hiding this comment

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

LG2M

@codecov
Copy link

codecov bot commented Feb 14, 2020

Codecov Report

Merging #1073 into master will increase coverage by 0.14%.
The diff coverage is 83.05%.

@@            Coverage Diff             @@
##           master    #1073      +/-   ##
==========================================
+ Coverage   66.26%   66.41%   +0.14%     
==========================================
  Files         147      145       -2     
  Lines       18791    18792       +1     
==========================================
+ Hits        12452    12480      +28     
+ Misses       6339     6312      -27
Impacted Files Coverage Δ
chain-abci/src/app/commit.rs 91.52% <ø> (+0.09%) ⬆️
chain-core/src/init/params.rs 77.77% <0%> (-3%) ⬇️
chain-abci/src/main.rs 0% <0%> (ø) ⬆️
chain-storage/src/tx.rs 93.1% <100%> (+6.89%) ⬆️
chain-abci/src/app/end_block.rs 100% <100%> (ø) ⬆️
chain-abci/src/app/app_init.rs 76.41% <100%> (+6.92%) ⬆️
test-common/src/chain_env.rs 94.01% <100%> (+0.02%) ⬆️
chain-storage/src/lib.rs 62.69% <14.28%> (-6.33%) ⬇️
enclave-protocol/src/lib.rs 68.93% <87.5%> (+11%) ⬆️
chain-abci/tests/tx_validation.rs 97.57% <90.9%> (-0.11%) ⬇️
... and 7 more

@tomtau tomtau force-pushed the fix/duplicate-storage branch from 6aa4cf3 to dfaaac9 Compare February 16, 2020 00:41
@tomtau
Copy link
Contributor Author

tomtau commented Feb 16, 2020

bors try

bors bot added a commit that referenced this pull request Feb 16, 2020
@lgtm-com
Copy link

lgtm-com bot commented Feb 16, 2020

This pull request introduces 2 alerts when merging dfaaac9 into 7c173ec - view on LGTM.com

new alerts:

  • 2 for Unused import

@tomtau
Copy link
Contributor Author

tomtau commented Feb 16, 2020

bors try

@bors
Copy link
Contributor

bors bot commented Feb 16, 2020

try

Already running a review

@bors
Copy link
Contributor

bors bot commented Feb 16, 2020

try

Build succeeded

@tomtau
Copy link
Contributor Author

tomtau commented Feb 16, 2020

bors try

bors bot added a commit that referenced this pull request Feb 16, 2020
…ixes crypto-com#866)

Solution: as a part of ADR-001: https://github.com/crypto-com/chain/blob/master/architecture-docs/adr-001.md
- tx-validation-app moved to chain-abci
- chain-abci build process expanded to handle SGX SDK steps for C stubs -- on non-Linux systems, it'd display a warning and compile the mock version
- tx-validation-app SGX unit test moved to chain-abci under a "sgx-test" feature flag
- tx-query-app test removed (it was a kind of "mini-integration" / functionality test that assumed a lot of old behaviour, and all of this is now tested in integration tests)
- enclave-bridge takes "intra enclave" requests that are passed directly to the ecalls and returns the response
- ZMQ server started in chain-abci in a separate thread to handle tx-query requests (note: tx-query was out of scope of ADR-001, as it doesn't have any storage -- its future is TBD depending on audit feedback etc.)
- redundant enclave protocol variants removed
- "readonly" storage version provided for serving tx-query requests -- rocksdb/kvdb is thread-safe... zmq server then takes the latest chain state or sealed transactions directly -- note: some fixes related to fees, enclave protocol etc. (other steps of ADR-001) would be addressed in a separate PR
- chain-abci storage expanded with one column for sealed transaction payloads
- integration test building and environment preparation updated

note: Makefile + chain-docs aren't updated yet / would be addressed in separate PRs
@tomtau tomtau force-pushed the fix/duplicate-storage branch from 98cfd01 to c98d677 Compare February 16, 2020 01:50
@tomtau
Copy link
Contributor Author

tomtau commented Feb 16, 2020

bors r+

bors bot added a commit that referenced this pull request Feb 16, 2020
1073: Problem: duplicate storage between chain-abci and tx-validation app (fixes #866) r=tomtau a=tomtau

Solution: as a part of ADR-001: https://github.com/crypto-com/chain/blob/master/architecture-docs/adr-001.md
- tx-validation-app moved to chain-abci
- chain-abci build process expanded to handle SGX SDK steps for C stubs -- on non-Linux systems, it'd display a warning and compile the mock version
- tx-validation-app SGX unit test moved to chain-abci under a "sgx-test" feature flag
- tx-query-app test removed (it was a kind of "mini-integration" / functionality test that assumed a lot of old behaviour, and all of this is now tested in integration tests)
- enclave-bridge takes "intra enclave" requests that are passed directly to the ecalls and returns the response
- ZMQ server started in chain-abci in a separate thread to handle tx-query requests (note: tx-query was out of scope of ADR-001, as it doesn't have any storage -- its future is TBD depending on audit feedback etc.)
- redundant enclave protocol variants removed
- "readonly" storage version provided for serving tx-query requests -- rocksdb/kvdb is thread-safe... zmq server then takes the latest chain state or sealed transactions directly -- note: some fixes related to fees, enclave protocol etc. (other steps of ADR-001) would be addressed in a separate PR
- chain-abci storage expanded with one column for sealed transaction payloads
- integration test building and environment preparation updated

note: Makefile + chain-docs aren't updated yet / would be addressed in separate PRs


Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Feb 16, 2020

try

Build succeeded

@bors
Copy link
Contributor

bors bot commented Feb 16, 2020

@bors bors bot merged commit c98d677 into crypto-com:master Feb 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants