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.

Sign up for GitHub

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

Fix #1128 #1129

Merged
merged 6 commits into from
Oct 30, 2019
Merged

Fix #1128 #1129

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ca30737
Fix #1128
igormcoelho Sep 30, 2019
ad790b3
Update BlockBase.cs
shargon Oct 15, 2019
b5072cc
Merge branch 'master' into igormcoelho-patch-1
shargon Oct 15, 2019
5189b3b
Merge branch 'master' into igormcoelho-patch-1
shargon Oct 18, 2019
baf7be8
Merge branch 'master' into igormcoelho-patch-1
lock9 Oct 29, 2019
a364255
Merge branch 'master' into igormcoelho-patch-1
vncoelho Oct 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions neo/Network/P2P/Payloads/BlockBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public UInt256 Hash
sizeof(ulong) + //Timestamp
sizeof(uint) + //Index
UInt160.Length + //NextConsensus
1 + //
1 + //Witness array count
Witness.Size; //Witness

Witness[] IVerifiable.Witnesses
Expand All @@ -59,8 +59,9 @@ Witness[] IVerifiable.Witnesses
public virtual void Deserialize(BinaryReader reader)
{
((IVerifiable)this).DeserializeUnsigned(reader);
if (reader.ReadByte() != 1) throw new FormatException();
Witness = reader.ReadSerializable<Witness>();
Witness[] witnesses = reader.ReadSerializableArray<Witness>(1);
Copy link
Member

Choose a reason for hiding this comment

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

Why not ?

            Witness witnesses = reader.ReadSerializable<Witness>();

Copy link
Contributor Author

@igormcoelho igormcoelho Oct 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We can do it.. the problem I see is only compatibility to JSON spec serialization. Will we change it to "witness" too? As long as we do it, it's fine for me. It's like having an inheritance that removed virtualization from Witnesses and exposed Witness. No logical problem I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree @shargon, no need to waste this byte for nothing. Lets update json spec too, to make it compatible.

Copy link
Member

Choose a reason for hiding this comment

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

No. We need to keep it as an array. So we can allow multiple witnesses in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Is not better to increase the version if we want to modify this in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is not better to increase the version if we want to modify this in the future?

That sounds good. Otherwise, why have a version 🤷‍♂

Copy link
Contributor Author

@igormcoelho igormcoelho Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Do you think there's hope for still using that array in blocks @erikzhang? I thought about it, and perhaps, there's no more applications for multi witness here on Neo3.... please correct me if I'm mistaken. I know you always has some clever tricks at hand.. cross chain maybe?
We don't mean removing from IVerifiable, only in BlockBase.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. If we support grouped consensus. And I think that even if we don't need multiple Witness in the future, we should serialize it according to the definition of the IVerifiable.Witnesses interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect. I fully agree with both justifications Erik.

if (witnesses.Length != 1) throw new FormatException();
Witness = witnesses[0];
}

void IVerifiable.DeserializeUnsigned(BinaryReader reader)
Expand All @@ -84,7 +85,7 @@ UInt160[] IVerifiable.GetScriptHashesForVerifying(Snapshot snapshot)
public virtual void Serialize(BinaryWriter writer)
{
((IVerifiable)this).SerializeUnsigned(writer);
writer.Write((byte)1); writer.Write(Witness);
writer.Write(new Witness[] { Witness });
}

void IVerifiable.SerializeUnsigned(BinaryWriter writer)
Expand Down