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

Local testnet implementation #1202

Merged
merged 94 commits into from
Jan 15, 2020
Merged

Local testnet implementation #1202

merged 94 commits into from
Jan 15, 2020

Conversation

lgalabru
Copy link
Contributor

@lgalabru lgalabru commented Jan 2, 2020

This PR is addressing #1180

Current status:
Looks functional 🎉.

With this PR, developers now have the ability to run a "testnet" on their machine.
The DX for using the testnet is a bit impractical at this point: developers need to generate signed transactions blobs using the command line (#1203), redirect the output in a file (binary format), and place the file in the mempool folder (set by the config).
The implementation of #1179 should improve the DX.

I think that we should also think about de-frosting https://github.com/blockstack/clarity-js-sdk (cc @zone117x) and start building a comprehensive flow for smart contract developers, that would cover tx signing + submission.

Concerning the implementation, in a nutshell, the testnet and its runloop is being setup with a Config struct, setting the average block time, the number of desired nodes, and their config (mempool path, etc).
Each node will maintain its chain state. When a node win a sortition, it will start a new LeaderTenure (also having its own chain state), and will start polling a mempool and building blocks / micro blocks.
At average_block_time / 2, the tenure will terminate, and return some artifacts (anchor-block, micro-blocks) that will be shared with the other nodes, so that they can submit their CommitBlockOp to the burnchain.

Current limitations that should not be blocking and that can be addressed in a separate PR (if that's ok with the team):

  • the leader should keep assembling micro-blocks after sharing the anchor-block, and keep sharing this new micro-blocks.
  • nodes keep submitting some LeaderKeyRegistrationOp on every block, even if they did not win sortition.
  • all the nodes are relying on the same burnchain simulator.

src/testnet/node.rs Outdated Show resolved Hide resolved
src/testnet/node.rs Outdated Show resolved Hide resolved
@jcnelson
Copy link
Member

The code looks good to me, but before merging, we should have good test coverage. Can you add some unit tests to do something like simulate a few rounds of the testnet with some sample transactions applied, and verify that the transactions all get mined? The tests should also cover mining invalid transactions -- both malformed transactions and transactions that throw runtime exceptions.

@lgalabru
Copy link
Contributor Author

Great, on it! thanks!

@lgalabru lgalabru self-assigned this Jan 13, 2020
src/testnet/tenure.rs Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This is awesome @lgalabru!

My only question is about the LeaderTenure implementation -- is there a reason the BlockBuilder::epoch_begin and BlockBuilder::epoch_end functions can't be used?

@@ -42,7 +42,7 @@ use util::hash::HASH160_ENCODED_SIZE;
use util::strings::StacksString;
use util::secp256k1::MessageSignature;

use address::AddressHashMode;
pub use address::AddressHashMode;
Copy link
Member

Choose a reason for hiding this comment

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

Why add this? Why not just import address::AddressHashMode in whatever module(s) that try to import AddressHashMode from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change coming from #1203 - addressed with 9b789be

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Thanks for taking care of it nevertheless 👍


assert!(&h_comp != &h_uncomp);
assert_eq!(h_comp.len(), 66);
assert_eq!(h_uncomp.len(), 64);
Copy link
Member

Choose a reason for hiding this comment

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

Can you also test that the final two characters in h_comp are 01, and that h_comp and h_uncomp are equal up to the 64th byte?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change coming from #1203 - addressed with cbdd3ac

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha. Thanks for fixing it nevertheless 👍

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

I just had a couple of small requests, but once they're merged in, this will LGTM. Please merge to develop once they're addressed. Great job on this @lgalabru!

@lgalabru lgalabru merged commit 71d8987 into develop Jan 15, 2020
@lgalabru lgalabru deleted the feature/testnet-runloop branch January 15, 2020 20:29
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