Spartan PoC Consensus: Milestone 1

[rg3l3dr]

Milestone Delivery Checklist

• The milestone-delivery-template.md has been copied and updated.
• The invoice form 📝 has been filled out for this milestone.
• This pull request is being made by the same account as the accepted application.
• In case of acceptance, the payment will be transferred to the initial BTC/ETH payment address.
• The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application PR: w3f/Grants-Program#357

[mmagician]

Thanks for the delivery, we'll review it as soon as possible!

[vedmakk]

Hello. Thanks again for your milestone 1 deliveries.

We've had a first look: The farmer plotted and - once the node was running - it started farming and I could watch the blocks being generated – so great work :)).

Overall your deliveries, documentation and articles are comprehensive and I just have a few remarks to consider:

General

• Please rename the delivery template of this PR to match our naming conventions: spartan_poc_consensus_module-milestone_**one**.md -> spartan_poc_consensus_module-milestone_**1**.md (just a detail, but it helps us keeping things clean).

spartan_client

Documentation:

• Since the spartan-farmer is decoupled and managed in a separate repository I suggest to also move all documentation regarding the setup and running of the farmer from the node-template-spartan README.md to the spartan-farmer repository and then just refer to these information. This avoids duplication and reduces the chance of outdated documentation. You already describe running the farmer differently in both readme's.
• The link to the "basic Rust setup instructions" is broken
• You might wanna remove the "Template Structure" section at the bottom as this is documentation for the general node-template and not specific to your implementation.

Docker:

• Despite taking a long time on OSX (as mentioned by you) the node-template-spartan image didn't compile in the end (the farmer image seemed fine though) so I couldn't test with Docker.

Code:

• The building process still spits out a few warnings - some related to code you've been adding.
• Please be consistent with Authors and Copyright declarations throughout the different modules. Sometimes you've added Parity as Co-Authors and sometimes you didn't (e.g. sp_consensus_spartan vs. sp_consensus_poc).

Functionality

• As mentioned above it plotted, farmed and generated blocks and I could monitor everything from substrate's front-end template client. Regarding the support of balance transfers – I was able to submit the transaction and the transfer was displayed as successful, but the account balances didn't update.

spartan_farmer

Documentation:

• I suggest to add a bit more clarity around the plotting process: Since you mention the plotting time to be around 36hours per TB… it would be good to mention that the 1GB test plot only takes a couple of seconds. Also I was wondering if the needed disk space during plotting could exceed the 1GB file size of the final plot (I remember that's the case with Chia Plotting) which would be useful to know.
• Not sure if this is applicable in your case, but Chia placed some warnings that intensive plotting would damage your SSD at some point and therefore plotting on your main drive is not recommended. If that holds true for the spartan-farmer it would be good to at least add a little warning (even though it's unlikely anyone goes hardcore plotting at this stage ;)).
• Once you've merged the docs around running the farmer, please be a bit more explicit about a running node being a prerequisite before the farmer can be started (especially since the farmer doesn't handle connection loss very well).
• Installation: cargo install spartan-farmer as mentioned in the docs resulted in an error; I had to use cargo +nightly install spartan-farmer
• This one is a bit opinionated: Although you did write comments, you've been a bit scarce on inline documentation within the source code of the spartan farmer compared to the code within substrate. A bit more documentation within the critical parts would help to understand what's going on more quickly.

Functionality:

Both plotting and farming works as intended under normal conditions - therefore the following inputs are probably optional for this milestone but might be considered later:

• Plotting: Once a plot has been created and you run the plot command again, it just exits without doing anything (even if you run it with different params). Maybe add a little hint that you need to erase any previously created plot, before you can create a new one.
• Farming: The farmer doesn't handle connection interruptions to the node currently and either fails to start or (when the connection is lost during operation) goes wild on console output and needs to be terminated.

sc_consensus_poc

• Is there a reason for leaving the tests in aux_schema.rs as a TODO - otherwise I'd kindly ask you to take care of them as well.

Thanks again for your great efforts and please feel free to ask any questions that pop-up and of course let me know if you have a different opinion on my remarks.

[rg3l3dr]

Thank you for taking the time to review our submission with such great detail @protyze . We appreciate all the feedback and comments. It's always good to know how other people see our project and code from the outside. We will be submitting a PR later today that addresses all of the issues you raised.

[rg3l3dr]

@protyze we have tried to address all of your issues with the above commits.

General

• Delivery template has been renamed to follow your conventions

spartan_client

• Docs for farmer now reside in farmer (expect for combined docker docs)
• Fixed linked to basic Rust setup in spartan_client
• We prefer to leave template structure in for consistency
• Docker has been completely overhauled. We have pushed pre-built images to a registry so you now only have to pull the images and run containers without having to build anything locally. It now works quickly on both Linux and Mac.
• The few warnings that remain from the build process were left as reminders to be resolved in later milestones, since they relate to imports for code that is currently commented out.
• Regarding module authors, we added Parity when we forked a module and made < 50% changes and we removed Parity when we had to do a full rewrite of the module (sp_consensus_spartan and sc_consenusus_poc_rpc). We retained whatever license had originally been used by Parity, though these were inconsistent in a few places.
• Regarding balance transfer updates in the browser, this task has been assigned to Milestone 2 (browser GUI updates).

spartan_farmer

• We have added more detail to the readme around the plotting process regarding plotting time, plot size, and potential damage to disk.
• We tried to be make the instructions more explicit about first running a node
• Rust nightly has been specified where needed now
• We have added more inline documentation to the farmer
• A hint has been added that you need to erase the plot before creating a new one
• Handling of connection drops has been improved

sc_consensus_poc

• The tests in aux_schema.rs are required for running migrations across the multiple versions of pallet_babe that have been deployed over its lifetime. Since we only have one version of spartan we cannot test this yet. We left it as a todo for future reference when we do need to support upgrades.

Thanks again for taking the time to review our code in detail and provide such great feedback. We have done our best to incorporate as much of it as possible.

[vedmakk]

Hi @rg3l3dr. Thank you very much for the adjustments. I will have a look into them and get back to you as soon as possible.

[vedmakk]

@rg3l3dr Thanks again for the swift reply. All your adjustments look very good to me and you have reasonably argued about the remarks that you wouldn't consider within this milestone. I'll suggest to accept your milestone 1 deliveries and a colleague will double check it before the final approval.

Two minor remarks regarding the adjusted README-files:

• The links in the README's in spartan-farmer and spartan-client are targeting specific tags or branches of each other and might link to old versions in the future.
• The spartan-farmer README now covers running the farmer using Docker and the process using cargo install. You might want to add the farmer-related parts of the section "Run in Development Mode" (using cargo build / cargo run) that has been previously described in the node-template-spartan README – just for completeness.

Thank you very much for all your efforts and the good work. I'm looking forward to see more of this project in the future!

[alxs]

Thank you @rg3l3dr for the delivery and @protyze for the evaluation. Looks quite good overall, nice work. I'll give it a deeper look and leave a few comments of my own in the coming 2-3 days.

[alxs]

@rg3l3dr apologies for the delay. I was a bit busier than expected. Great delivery overall, congratulations on what you've achieved so far and thanks for the adjustments you've already made. I have a few remarks to add to @protyze's already thorough review:

• On the forked Substrate repository containing the client functionality: it is currently pretty hard for an outsider to understand the purpose of this repo without access to the milestone delivery documentation. It looks like just another Substrate fork, which is unfortunate as you don't get to showcase the work you've done. Ideally you would:
  • Rename it to spartan-client, spartan or whatever you feel like that accurately describes its purpose.
  • Update the main README to briefly describe the purpose of the repo, link to the components you've implemented and describe how they interact with one another, and link to the spartan-farmer, spartan-codec and other relevant repositories. As I see it, this is now the main repository for your project—so it'd be just the right place to lay out to technical folks what you're working on. You may see some interest from the community soon (and should, IMO), so this would be in your best interest.
  • Improve on the documentation of the components you've implemented. All these READMEs are mostly copy-pasted. Ideally you'd use them to describe what each of the modules do. The last one in specific would ideally not replicate information pertaining to BABE but only describe the differences to it—you can always refer to the BABE one. Feel free to also just remove the READMEs and describe these components in the main one.
  • Remove all components you don't need. I understand this may take some work, but unless you plan to eventually move over to another repository for the client (which you may want to consider), everything that isn't strictly required for your project only pollutes your implementation.
  • FRAME is simply the name for the set of pallets provided alongside Substrate that make runtime development easier. Meaning, the spartan pallet shouldn't be in that folder. FRAME pallets you depend on should also be included as dependencies instead of within the repository.
  • I suggest to rename node-template-spartan to spartan-node or the like, as this isn't a template any more.
• As you've seen Jeff Burdges, one of our researchers, left some comments on your application (Subspace Grants-Program#357 (comment)). We have some concerns that Subspace may not be fully compatible as a parachain, which I assume you'd want to clarify sooner rather than later. An open discussion especially regarding anything relevant to the grant and parachain eligibility is ideal to keep us in the loop, but regardless of this you can reach him on Element under @jeff:web3.foundation.
• spartan-farmer lacks the Apache 2.0 license.
• It'd be great if you could update the links in your documentation to not target the milestone tag, as Jan mentioned in his last comment.
• How is your application to the Builders Program going?
• We cross-promote announcements about milestone deliveries on the second Monday of each month, such as the Medium article you delivered. If you're interested and you haven't done so yet, feel free to reach out to grantsPR@web3.foundation.
• Love the sloth ;)

As I said, independently of my comments this is all in all A+ work. Keep it up and let us know if you need anything else.

[rg3l3dr]

@alxs thanks for the detailed review and positive feedback. Most of the points you raise were either deliberate design decisions on our part or us seeking to replicate the style and structure of the substrate modules this work was based on.

Regarding your comments, we will be submitting the necessary changes soon, but I wanted to quickly provide our thought process so we can come to consensus on what those changes should actually be.

• Agreed that it is not clear what was changed when starting from the top level readme in our fork of Substrate, we will update that readme accordingly. We have instead used the node-template-spartan readme as the entry point for our docs and guides, which provides the background info you described, but that is not immediately obvious from the outside.
• We would like to keep the structure of our fork as close to substrate as possible so that we may continue to pull in upstream changes and (hopefully) eventually merge some of these changes back into master.
• The documentation for the components we implemented follows the same patterns as employed by Parity for the babe related crates, i.e. we generally provided no more / no less documentation than was provided by the original module which we forked. We did revise sc_consensus_poc, if it appears similar to sc_consensus_babe that is because both protocols are based on the same notion of time as employed by Ouroboros Praos, though we replace the VRF with a PoR. This was the main reason we chose to fork babe, as opposed to writing our own pallet from scratch.
• We included spartan in the frame directory since this is how babe was structured, we want this to be a generic pallet just like babe, that can be implemented for a generic template, as such node-template-spartan is just a template that implements spartan similar to how node-template is a template that implements aura and grandpa.
• We reached out to the researcher who left the comments regarding the incompatibility of proof-of-space consensus with polkadot. After we have had a chance to talk with him further and make sure we understand each other clearly we will post an update as to if this is actually a problem or how we intend to resolve it.
• We will include the Apache 2.0 license with spartan-farmer
• The documentation links intentionally target the milestone tags as we plan to have a different branch of the project for each milestone to clearly see changes and progression. As we progress through milestones (and branches) we will update the links accordingly.
• We have applied to builders and completed the initial screening interview. We will be submitting the full packet to start the committee review once this milestone is fully accepted, as we were told that is a pre-requisite for consideration.
• We will be reaching out to the grants rep for cross promotion. I must confess that I made a mistake earlier today and misread comments between this milestone PR and the original grant PR as indicating it had been accepted, which led us to post the medium article and make some announcements on social media. After re-reading your comments I realize that was done in error, and that won't happen again.
• Glad you like the sloth ascii art, that was actually ported from the original Python project created by @mathiassmichno for RandomChain, it was so cool we had to include it!

I hope that all makes sense. These decisions were based on several iterations of ways to structure the project and this ended up working the best for us to maintain compatibility as Substrate evolves.

[mmagician]

@rg3l3dr Just to add my 5 cents here:

We included spartan in the frame directory since this is how babe was structured, we want this to be a generic pallet just like babe, that can be implemented for a generic template, as such node-template-spartan is just a template that implements spartan similar to how node-template is a template that implements aura and grandpa.

That makes partial sense to me. I'm not an expert, yet I believe you might be mixing up two concepts here.

If I understand correctly and you want to become a parachain, then your codebase should most likely not be based off the substrate repo, but rather have a structure similar to the node template or even the polkadot repo. Think of the substrate repo as a collection of building blocks, and the polkadot repo as a specific instantiation of these blocks with one concrete configuration. I think that your spartan node has more parallels with polkadot than with substrate in the above comparison - am I right in thinking this?

We would like to keep the structure of our fork as close to substrate as possible so that we may continue to pull in upstream changes and (hopefully) eventually merge some of these changes back into master.

Keeping the above in mind, you can notice that if you have a specific instantiation of substrate (akin to polkadot) then you can still keep referring to the latest changes in substrate (e.g. see how polkadot references primitives).

If you have merging upstream at some point in mind, you might want to find the middle ground, and keep the substrate fork for your primitives and FRAME-to-be pallets, and then fork off substrate-node-template repo and reference your substrate fork for any custom pallets. This way you avoid coupling the code for your node with the core, unchanged substrate code.

Let me know if this makes sense.

[nazar-pc]

We believe that it is more similar to Substrate repo than Polkadot and that is why we started with Substrate fork.

First of all, we need some new primitives and we also needed changes to some of substrate's components to make things work (specifically sp-core and substrate-test-runtime).
We also wanted to be fairly up to date, so we had to start with substrate fork from master branch as it was at the time with ability to merge upstream changes and keep things up to date.

Second, we do not intend to launch Spartan as a full-fledged network on its own. It is a preparation, building of components for Subspace and potentially other PoC-based protocols on Substrate.
Hence we see node-template-spartan based on Spartan consensus being directly analogous to bin/node-template in substrate's repo that is based on Aura (and bin/node, which also looks like a node template, but based on BABE), that others can clone and start modifying.

Third, we build those components in such a way that with click of a button we can create a PR into upstream.
We do not know if upstream would want to accept it, but the approach was intentially selected such that makes it possible, we wouldn't want to maintain our fork separately from the primary concentration of the effort (that is upstream Substrate) forever.

And lastly, targeting a particualr snapshot of upstream master branch feels like counter-productive and anti-pattern.
Ideally production chains should be built against stable versions of the components released to crates.io and not against random (or deliberate) snapshot of the ever changing upstream that doesn't have a meaningful semver version assigned to it.
Of course, Subspace will have its own repo similar to Polkadot and will contain purpose-built components that we do not plan to upstream, and your suggestions are directly applicable there.

TL;DR: In a perfect world eventual result would be such that cargo install node-template-spartan just works with stable versions of upstream components all available on crates.io.

[mmagician]

Thanks for elaborating on your reasoning, that makes sense 👍🏼

[alxs]

Thanks for being so responsive @rg3l3dr @nazar-pc. Regarding the inclusion within FRAME and/or Substrate, feel free to create an issue or draft PR upstream to see if there is indeed interest to merge this at some point in the future. However, note that we had this situation with another grantee recently and Parity wasn't very keen to merge their code. See paritytech/substrate#7820 (comment). The largest issue is that maintaining an external pallet, and even more so another consensus algorithm, would require developer resources from Parity's side which are already strained.

In general, while I agree with most of your arguments, I think apart from this point it'd still be a good idea to work on your own repository instead of the current fork. But I suggest we discuss the reasons for this on Element; I've created a room with the two of you in which to discuss this freely. Feel free to wait with this until the integration within Substrate has been clarified.

No worries about the public announcements, I can see how the external review outside of the standard review process may have been confusing. In any case, I think there's not much you could do at this point for this milestone to be rejected ;). Let me know when you're done with the changes and I'll be happy to merge the PR.

[rg3l3dr]

@mmagician @alxs thanks again for the detailed feedback and comments. The final remaining issues should now be addressed and the PR should be ready to merge.

• We have added the Apache 2.0 license to spartan-farmer
• We have updated the root readme of subspace/substrate to explain that this is a fork and point to the actual entrypoint.
• We have discussed @burdges issue offline with him (regarding PoC consensus on Polkadot) and the TL;DR is that since our protocol is Praos-like it should be compatible with the current architecture, since farmers (collators) could submit proofs that they can create a block for some upcoming slot, and connect to a relay chain validator without opening them up to a DoS attack.
• We have also discussed the finer details of consensus integration with @rphmeier and now understand that we will eventually need to make some decisions regarding whether we wish to be a parachain, parathread, or sovereign chain connected via a bridge. In the current state of the project, it works as an independent chain with its own block production mechanism and fork choice rule. If we wish it to be a parachain, we will need to remove the fork-choice rule (delegating this to the relay chain) and simplify farmers (collators) so that they only produce new blocks.
• To be clear, the above design decisions are outside the scope of this grant, though they need to be figured out before we can properly integrate into the Polkadot ecosystem. Now that this milestone has been approved we will be submitting our Substrate Builders app soon, and hope to get more feedback on these challenges through that program as well.

We have also nearly completed all the tasks for milestone 2 of this grant and plan to submit that next week.

[alxs]

Thank you for the updates! Regarding the license in spartan-farmer, the project still doesn't actually contain one. If you're happy with both MIT or Apache 2.0, you can just add a LICENSE file for either of them to the repository.

We have also discussed the finer details of consensus integration with @rphmeier and now understand that we will eventually need to make some decisions regarding whether we wish to be a parachain, parathread, or sovereign chain connected via a bridge. In the current state of the project, it works as an independent chain with its own block production mechanism and fork choice rule. If we wish it to be a parachain, we will need to remove the fork-choice rule (delegating this to the relay chain) and simplify farmers (collators) so that they only produce new blocks.

Sounds good, keep us updated in this regard. Also just curious, how would this work? I had a look at your Subspace paper a few weeks ago and if I remember correctly, farmers were the PoC-based nodes of the network, which I'm assuming might face the same issues as a PoW chain, whereas executors formed a PoS layer on top of those. If as I understood it the purpose of the two separate node tiers was to solve the farmer's dilemma, can you still do so while removing the executor role? Regardless of this, how would you roughly approach combining Spartan and Ouroboros into one consensus algorithm without this separation?

Besides, do you have any updates regarding merging into Substrate?

In any case, I'm happy to tell you that the milestone is a pass! You can find my evaluation notes here. I'll forward your invoice internally.

[rg3l3dr]

Thanks @alxs!

We misunderstood on the license, thinking it just needed to be declared in the cargo.toml, but we will add an actual license file in the next milestone.

Actually the farmer PoC protocol is praos-like (not the executor staking election), in that we divide time into fixed slots and epochs and then derive the randomness for the current epoch from some past epoch. Farmer's can then look ahead into the current epoch to see if they are elected, and use this proof as the basis for connecting to a relay chain validator. Executors are better understood as a finalization or ratification mechanism on top of the farmer protocol and they are chosen for each new block (not in a praos-like fashion). But they are not providing "finality" just proposing a new state root that can be challenged within some multiple of the network delay.

In order to integrate directly as a parachain we will need remove the chain selection rule and just have farmers produce blocks, while relying on the relay chain to choose the longest chain (currently it does this internally). In that cases executors would work off the longest relay chain branch. We could also have our own finality gadget (such as Taiji) and have only farmed, executed, and finalized blocks sent to the relay chain (as a parachain). Others options are to to retain the current structure and run as a parthread or build a bridge into Polkadot as a fully sovereign chain. Ideally we would like to be parachain while still being only loosely coupled to the relay chain from a consensus perspective, though we're not sure if that is possible within the current architecture. Still a lot of thinking and discussion to do here on the best way to integrate, and it will probably come down to what works best with XCMP so that we can provide efficient cross-parachain storage within Polkadot.

No updates regarding merging into Substrate yet, but hoping to get more direction on that through Substrate Builders, we should be submitting our final application this weekend.

Thanks for all the great feedback and facilitating the discussion!

[RouvenP]

hi @rg3l3dr we transferred the payment.