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 Witness limits #1958

Merged
merged 7 commits into from
Oct 3, 2020
Merged

Fix Witness limits #1958

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
55db297
Fix Witness max size
shargon Sep 24, 2020
b3ca6ac
Fix comment
shargon Sep 24, 2020
68e2e03
Fix comment
shargon Sep 24, 2020
033fd75
Merge branch 'master' into fix-witness
shargon Sep 26, 2020
33ce6de
Change it to use constants
shargon Oct 1, 2020
113b64e
Changed to 1024
shargon Oct 1, 2020
e3cb0ed
Fix UT
shargon Oct 1, 2020
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
10 changes: 5 additions & 5 deletions src/neo/Network/P2P/Payloads/Witness.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ public virtual UInt160 ScriptHash

void ISerializable.Deserialize(BinaryReader reader)
{
// This is designed to allow a MultiSig 10/10 (around 1003 bytes) ~1024 bytes
// Invocation = 10 * 64 + 10 = 650 ~ 664 (exact is 653)
InvocationScript = reader.ReadVarBytes(663);
// Verification = 10 * 33 + 10 = 340 ~ 360 (exact is 351)
VerificationScript = reader.ReadVarBytes(361);
// This is designed to allow a MultiSig 21/11 (committee)
// Invocation = 11 * (64 + 2) = 726
InvocationScript = reader.ReadVarBytes(744);
// Verification = m + (PUSH_PubKey * 21) + length + null + syscall = 1 + ((2 + 33) * 21) + 2 + 1 + 5 = 744
VerificationScript = reader.ReadVarBytes(760);
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem to be too perfectly calculated to me. I'd rather have some simpler values here like 2048 or 4096 so that we would have at least some reserve for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

1024 and 1024 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have any strong opinion here. 2048 seems to be a bit more future-proof to me, but 1024 is already better than 744/760.

}

void ISerializable.Serialize(BinaryWriter writer)
Expand Down
27 changes: 17 additions & 10 deletions tests/neo.UnitTests/Network/P2P/Payloads/UT_Witness.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,13 @@ public void InvocationScript_Get()
uut.InvocationScript.Should().BeNull();
}

private Witness PrepareDummyWitness(int maxAccounts)
private Witness PrepareDummyWitness(int pubKeys, int m)
{
var address = new WalletAccount[maxAccounts];
var wallets = new NEP6Wallet[maxAccounts];
var walletsUnlocks = new IDisposable[maxAccounts];
var address = new WalletAccount[pubKeys];
var wallets = new NEP6Wallet[pubKeys];
var walletsUnlocks = new IDisposable[pubKeys];

for (int x = 0; x < maxAccounts; x++)
for (int x = 0; x < pubKeys; x++)
{
wallets[x] = TestUtils.GenerateTestWallet();
walletsUnlocks[x] = wallets[x].Unlock("123");
Expand All @@ -43,9 +43,9 @@ private Witness PrepareDummyWitness(int maxAccounts)

// Generate multisignature

var multiSignContract = Contract.CreateMultiSigContract(maxAccounts, address.Select(a => a.GetKey().PublicKey).ToArray());
var multiSignContract = Contract.CreateMultiSigContract(m, address.Select(a => a.GetKey().PublicKey).ToArray());

for (int x = 0; x < maxAccounts; x++)
for (int x = 0; x < pubKeys; x++)
{
wallets[x].CreateAccount(multiSignContract, address[x].GetKey());
}
Expand All @@ -69,7 +69,7 @@ private Witness PrepareDummyWitness(int maxAccounts)
Witnesses = new Witness[0]
});

for (int x = 0; x < maxAccounts; x++)
for (int x = 0; x < m; x++)
{
Assert.IsTrue(wallets[x].Sign(data));
}
Expand All @@ -81,7 +81,7 @@ private Witness PrepareDummyWitness(int maxAccounts)
[TestMethod]
public void MaxSize_OK()
{
var witness = PrepareDummyWitness(10);
var witness = PrepareDummyWitness(10, 10);

// Check max size

Expand All @@ -100,7 +100,14 @@ public void MaxSize_OK()
[TestMethod]
public void MaxSize_Error()
{
var witness = PrepareDummyWitness(11);
var witness = PrepareDummyWitness(21, 12);

// Check max size

Assert.IsTrue(witness.Size > 1024);
Assert.ThrowsException<FormatException>(() => witness.ToArray().AsSerializable<Witness>());

witness = PrepareDummyWitness(22, 11);

// Check max size

Expand Down