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 GetSigOpCount and revert earlier workaround #493

Merged
merged 3 commits into from
Apr 6, 2021
Merged

Fix GetSigOpCount and revert earlier workaround #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bbc91e
Fix GetSigOpCount and revert earlier workaround
quantumagi Mar 31, 2021
0bbec4c
Add network argument
quantumagi Mar 31, 2021
530a568
Add network argument
quantumagi Mar 31, 2021
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
20 changes: 20 additions & 0 deletions src/NBitcoin.Tests/sigopcount_tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Linq;
using Moq;
using Stratis.Bitcoin.Tests.Common;
using Xunit;

Expand Down Expand Up @@ -39,5 +40,24 @@ public void GetSigOpCount()
scriptSig2 = scriptSig2 + OpcodeType.OP_1 + dummy.ToBytes() + dummy.ToBytes() + s2.ToBytes();
Assert.Equal(3U, p2sh.GetSigOpCount(KnownNetworks.Main, scriptSig2));
}

[Fact]
[Trait("Core", "Core")]
public void GetSigOpCountForFederation()
{
PubKey[] keys = Enumerable.Range(0, 3).Select(_ => new Key(true).PubKey).ToArray();
var federations = new Federations();
federations.RegisterFederation(new Federation(keys.Take(2), 1));
var network = KnownNetworks.StraxRegTest;
network.SetPrivatePropertyValue("Federations", federations);

// Test CScript::GetSigOpCount()
var s1 = new Script();
s1 = s1 + OpcodeType.OP_1 + OpcodeType.OP_FEDERATION + OpcodeType.OP_CHECKMULTISIG;
Assert.Equal(2U, s1.GetSigOpCount(true, network));
s1 = s1 + OpcodeType.OP_IF + OpcodeType.OP_CHECKSIG + OpcodeType.OP_ENDIF;
Assert.Equal(3U, s1.GetSigOpCount(true, network));
Assert.Equal(21U, s1.GetSigOpCount(false, network));
}
}
}
10 changes: 6 additions & 4 deletions src/NBitcoin/Script.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ public IList<Op> ToOps()
}
}

public uint GetSigOpCount(bool fAccurate)
public uint GetSigOpCount(bool fAccurate, Network network = null)
{
uint n = 0;
Op lastOpcode = null;
Expand All @@ -865,7 +865,9 @@ public uint GetSigOpCount(bool fAccurate)
n++;
else if (op.Code == OpcodeType.OP_CHECKMULTISIG || op.Code == OpcodeType.OP_CHECKMULTISIGVERIFY)
{
if (fAccurate && lastOpcode != null && lastOpcode.Code >= OpcodeType.OP_1 && lastOpcode.Code <= OpcodeType.OP_16)
if (fAccurate && network?.Federations != null && lastOpcode.Code == OpcodeType.OP_FEDERATION)
n += (uint)network.Federations.GetOnlyFederation().GetFederationDetails().transactionSigningKeys.Length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose that, strictly speaking, this uses how many members there are in the federation in total rather than the number that actually signed a given transaction. So the computed sigOp count will land up somewhat higher than it could be.

Copy link
Contributor Author

@quantumagi quantumagi Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I assume that would be consistent with the existing behaviour for non-federation multi sig below which also does not use the actual number of signatures - only the number accepted by the script. E.g. looking at:
s1 = s1 + OpcodeType.OP_1 + dummy.ToBytes() + dummy.ToBytes() + OpcodeType.OP_2 + OpcodeType.OP_CHECKMULTISIG;
The number just preceding the OP_CHECKMULTISIG would be the max number of signatures that CAN be passed.

else if (fAccurate && lastOpcode != null && lastOpcode.Code >= OpcodeType.OP_1 && lastOpcode.Code <= OpcodeType.OP_16)
n += (lastOpcode.PushData == null || lastOpcode.PushData.Length == 0) ? 0U : (uint)lastOpcode.PushData[0];
else
n += 20;
Expand Down Expand Up @@ -914,12 +916,12 @@ public uint GetSigOpCount(Network network, Script scriptSig)
{
// TODO: Is the network needed?
if (!IsPayToScriptHash(network))
return GetSigOpCount(true);
return GetSigOpCount(true, network);
// This is a pay-to-script-hash scriptPubKey;
// get the last item that the scriptSig
// pushes onto the stack:
bool validSig = new PayToScriptHashTemplate().CheckScriptSig(network, scriptSig, this);
return !validSig ? 0 : new Script(scriptSig.ToOps().Last().PushData).GetSigOpCount(true);
return !validSig ? 0 : new Script(scriptSig.ToOps().Last().PushData).GetSigOpCount(true, network);
// ... and return its opcount:
}

Expand Down
2 changes: 1 addition & 1 deletion src/Stratis.Bitcoin.Features.Consensus/Rules/CommonRules/CoinviewRule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ private long CountWitnessSignatureOperation(Script scriptPubKey, WitScript witne
if (witParams.Program.Length == 32 && witness.PushCount > 0)
{
Script subscript = Script.FromBytesUnsafe(witness.GetUnsafePush(witness.PushCount - 1));
return subscript.GetSigOpCount(true);
return subscript.GetSigOpCount(true, this.Parent.Network);
}
}

Expand Down
9 changes: 3 additions & 6 deletions src/Stratis.Bitcoin.Features.MemoryPool/Rules/CreateMempoolEntryMempoolRule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,13 +234,10 @@ private bool AreInputsStandard(Network network, Transaction tx, MempoolCoinView
var redeemScript = new Script(ctx.Stack.Top(-1));

// TODO: Move this into a network-specific rule so that it only applies to Strax (the Cirrus validator already allows non-standard transactions)
if (!redeemScript.ToOps().Select(o => o.Code).Contains(OpcodeType.OP_FEDERATION))
if (redeemScript.GetSigOpCount(true, this.network) > MaxP2SHSigOps)
{
if (redeemScript.GetSigOpCount(true) > MaxP2SHSigOps)
{
this.logger.LogTrace("(-)[SIG_OP_MAX]:false");
return false;
}
this.logger.LogTrace("(-)[SIG_OP_MAX]:false");
return false;
}
}
}
Expand Down