-
Notifications
You must be signed in to change notification settings - Fork 170
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
P2P networking support for bootstrapping with UTXO set snapshot #1444
Conversation
…-set-bootstrapping
…otDownloadPlan, scaladocs
@@ -13,8 +12,8 @@ class ManifestSerializer(manifestDepth: Byte) extends ErgoSerializer[BatchAVLPro | |||
private val nodeSerializer = VersionedLDBAVLStorage.noStoreSerializer | |||
|
|||
override def serialize(manifest: BatchAVLProverManifest[DigestType], w: Writer): Unit = { | |||
val height = manifest.rootHeight | |||
w.putBytes(Ints.toByteArray(height)) | |||
val rootNodeHeight = manifest.rootHeight.toByte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be toByteExact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, by construction AVL+ tree has height <= 127 , then manifest has the same limitation also. Need to fix manifest in scrypto about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it makes more assumption about input data. If however there is a mistake in the data, the silent overflow will lead to hard to localize bugs. Why not to do checks here? It will not impact performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There MUST BE NOT mistakes here, if prover is having root height > 127 it can not work with the tree simply, forming bad manifest is very secondary issue then. Even more, height is much less than 127 always, there is a descriptive comment in scrypto about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I just curious why you think not checking overflow is better than checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it could be more confusing actually for a reader to find a check for a thing which MUST NOT happen by construction. Conversion to int in some manifest / db classes is already confusing.
avldb/src/main/scala/scorex/crypto/authds/avltree/batch/VersionedLDBAVLStorage.scala
Show resolved
Hide resolved
src/main/scala/org/ergoplatform/network/ErgoNodeViewSynchronizer.scala
Outdated
Show resolved
Hide resolved
src/main/scala/org/ergoplatform/settings/NodeConfigurationSettings.scala
Show resolved
Hide resolved
src/main/scala/org/ergoplatform/settings/NodeConfigurationSettings.scala
Show resolved
Hide resolved
Eliminate insert(values: Array[(K, V)]) in LDBKVStore
… into utxo-set-bootstrapping
@aslesarenko added manifest verification also in f4b2654 |
This PR implements P2P networking protocol for bootstrapping with UTXO set snapshot. The final big chunk of the code related to the new functionality.