Skip to content

all: switch from merkle tree to serial hash#6

Open
tuxcanfly wants to merge 71 commits intokyokan:masterfrom
tuxcanfly:switch-serial-hash
Open

all: switch from merkle tree to serial hash#6
tuxcanfly wants to merge 71 commits intokyokan:masterfrom
tuxcanfly:switch-serial-hash

Conversation

@tuxcanfly
Copy link

@tuxcanfly tuxcanfly commented Jan 16, 2021

This is a protocol level change which switches away from the random-write and merkle tree based approach to a append-only serial hash method.

The main benefit of this switch is the simplified sync protocol and less code complexity. As an example, the sync can be done in fewer messages and larger chunks of sectors now that the blob is guaranteed to retain data written earlier as it is now append-only.

N̶o̶t̶e̶ ̶t̶o̶ ̶r̶e̶v̶i̶e̶w̶e̶r̶s̶:̶ ̶I̶ ̶h̶a̶v̶e̶ ̶t̶r̶i̶e̶d̶ ̶t̶o̶ ̶k̶e̶e̶p̶ ̶t̶h̶i̶s̶ ̶c̶h̶a̶n̶g̶e̶s̶e̶t̶ ̶a̶s̶ ̶c̶o̶m̶p̶a̶c̶t̶ ̶a̶s̶ ̶p̶o̶s̶s̶i̶b̶l̶e̶ ̶h̶o̶w̶e̶v̶e̶r̶,̶ ̶s̶o̶m̶e̶ ̶u̶n̶r̶e̶l̶a̶t̶e̶d̶ ̶c̶h̶a̶n̶g̶e̶s̶ ̶m̶i̶g̶h̶t̶ ̶h̶a̶v̶e̶ ̶c̶r̶e̶p̶t̶ ̶i̶n̶.̶ ̶P̶l̶e̶a̶s̶e̶ ̶i̶g̶n̶o̶r̶e̶ ̶a̶n̶y̶t̶h̶i̶n̶g̶ ̶t̶h̶a̶t̶ ̶d̶o̶e̶s̶ ̶n̶o̶t̶ ̶l̶o̶o̶k̶ ̶r̶e̶l̶e̶v̶a̶n̶t̶ ̶t̶o̶ ̶t̶h̶e̶ ̶t̶o̶p̶i̶c̶.̶ ̶A̶l̶s̶o̶,̶ ̶I̶ ̶h̶a̶v̶e̶ ̶n̶o̶t̶ ̶m̶o̶d̶i̶f̶i̶e̶d̶ ̶t̶h̶e̶ ̶e̶x̶e̶c̶u̶t̶a̶b̶l̶e̶s̶ ̶a̶n̶d̶ ̶i̶n̶s̶t̶e̶a̶d̶ ̶a̶d̶d̶e̶d̶ ̶a̶ ̶n̶e̶w̶ ̶p̶a̶i̶r̶ ̶(̶̶d̶d̶r̶p̶d̶̶,̶ ̶̶d̶d̶r̶p̶c̶l̶i̶̶)̶ ̶i̶n̶s̶t̶e̶a̶d̶.̶ ̶S̶o̶m̶e̶ ̶o̶f̶ ̶t̶h̶e̶ ̶t̶e̶s̶t̶s̶ ̶a̶r̶e̶ ̶f̶a̶i̶l̶i̶n̶g̶ ̶a̶n̶d̶ ̶w̶i̶l̶l̶ ̶b̶e̶ ̶f̶i̶x̶e̶d̶ ̶b̶e̶f̶o̶r̶e̶ ̶f̶i̶n̶a̶l̶i̶z̶i̶n̶g̶ ̶t̶h̶i̶s̶ ̶P̶R̶.̶

Converted to Draft and added a pending list of TODO items before this is ready.

@mslipper
Copy link
Contributor

Please change the package name back to fnd, and remove references to fnd.localhost.

@mslipper
Copy link
Contributor

We also don't need to vendor dwire anymore.

@tuxcanfly
Copy link
Author

Ok I will rename it back to fnd. I would need to vendor dwire though, as I need to bump up the DefaultMaxByteFieldLen, which is required for BlobRes.Payload in the worst case.

@tuxcanfly tuxcanfly marked this pull request as draft January 17, 2021 05:55
@tuxcanfly
Copy link
Author

tuxcanfly commented Jan 17, 2021

TODO:

  • Fix tests around blob io
  • B̶u̶m̶p̶ ̶u̶p̶ ̶M̶a̶x̶ ̶B̶l̶o̶b̶ ̶S̶i̶z̶e̶ ̶(̶1̶M̶B̶ ̶-̶>̶ ̶8̶M̶B̶?̶)̶
  • Clean up stray references to ddrp/fnd and vice-versa
  • Tidy up diff

Not touching blob size at this time.

lgr.Trace("received unsolicited sector", "sector_id", msg.SectorID, "peer_id", peerID)
continue
var sectorTipHash crypto.Hash = opts.PrevHash
for i := 0; int(i) < len(msg.Payload); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to cast int here?

if err := opts.Tx.WriteSector(msg.SectorID, msg.Sector); err != nil {
lgr.Error("failed to write sector", "sector_id", msg.SectorID, "err", err)
continue
for i := 0; int(i) < len(msg.Payload); i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

}

if entry.Timestamp.After(update.Timestamp) {
if entry.SectorSize > update.SectorSize {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably rename the error if it's a misnomer now.

if entry.SectorSize > update.SectorSize {
return ErrUpdateQueueStaleTimestamp
}
if entry.Signature != update.Signature {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still possible?

@mslipper
Copy link
Contributor

Given the number of "cruft" changes (like reordering imports and renaming things) it's pretty hard for me to review the parts of this PR that are material to the protocol. Let's walk through it on Monday/Tuesday when we meet.

@tuxcanfly
Copy link
Author

Sorry about that I didn't have much time to clean up when copying over the changes from my non-git codebase, unfortunately. I will clean up and tidy things so we can review the changes that are actually important.

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.

2 participants