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.

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

Merge transaction's Sender and Cosigners on the wire #1747

Closed
roman-khimov opened this issue Jul 3, 2020 · 3 comments · Fixed by #1752
Closed

Merge transaction's Sender and Cosigners on the wire #1747

roman-khimov opened this issue Jul 3, 2020 · 3 comments · Fixed by #1752
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Summary

We can improve network efficiency and processing speed by merging Sender and Cosigners into one wire-level field. Note that this is not about removing the concept of Sender or Cosigners, it's just about P2P protocol level (wire format) optimizations.

Current state

As of preview2 both Sender and Cosigners were first-class fields of Transaction:

            Sender = reader.ReadSerializable<UInt160>();
...
            Cosigners = reader.ReadSerializableArray<Cosigner>(MaxCosigners);

Then after preview2 cosigners were moved to attributes with #1620 and it's a bit harder to trace their decoding, but the end result of it looks like this (we need to filter attributes to get cosigners):

        public IReadOnlyDictionary<UInt160, Cosigner> Cosigners => _cosigners ??= attributes.OfType<Cosigner>().ToDictionary(p => p.Account);

So as of preview2 a sender occupied 20 bytes in transaction, cosigners added one byte for array length (independent of their real presence) and a minimum of 21 bytes (hash and flag) per cosigner.

After #1620 a sender still uses 20 bytes, there is nothing added when there are no cosigners present and any real cosigner now occupies at least 22 bytes (21 for cosigner and 1 for attribute type.

In #1620 I've already noted that it might be a suboptimal choice, but now I think it can be proven with preview2 testnet data.

Preview2 testnet statistics

I've used a dump of preview2 testnet blocks at height of 304060 for this analysis, so you can recheck it. In these 304061 blocks (including genesis) there are just 269 transactions. Each of them obviously has a Sender, but we're more interested in cosigners, so out of them 117 have no cosigners at all, 150 have one cosigner and 2 transactions have two cosigners. From 152 transactions that use cosigners 151 have sender's hash as one of cosigners.

So it's 57% of cosigner-enabled transactions vs 43% non-cosigned. It's not the ratio I would expect, so I've dug a bit into these 117 non-cosigned transactions and I can see that 1 of these is obviously from the genesis block, it's special, 16 failed on execution (and thus are not very relevant, most probably these failures were even intentional as it's a testnet) and 51 of them do deploy a new contract on the network.

These 51 contract-deploying transactions come within just 269 overall transactions which is obviously not a very representative ratio for a real network. Neo 2 mainnet with its 30M+ transactions in 5.7M blocks (even if we're to forget about miner transations that would be 24M+) has just around 200 contracts deployed.

Now 151 of 152 cosigned transactions have duplicate hashes in Sender and one of Cosigners. That's exactly what I've expected to see, again, in a real network that ratio is expected to be even closer to 1.

So we can count now how much space these fields occupy in transactions (and databases): 269 * (20 + 1) + 150 * 21 + 2 * 21 * 2 = 8883, that's for preview2-encoding. If we're to switch to post-preview2 that would be: 269 * 20 + (150 * 22) + (2 * 22 * 2) = 8768, so there is a little improvement for this transaction set, but I think we can expect more cosigned transactions on a real network.

Proposed changes

Remove explicit Sender from transaction's serialized representation, remove Cosigner attributes, add Signers array to transaction which would be an array of cosigners as they're currently defined. No new cosigner flags should be added, the Sender is the first Signer in the list of Signers. If there is a need to emulate current Sender's behavior (which just pays the bills and does not represent a Cosigner) an additional None WitnessScope could be added which would mean that this witness has no scope.

That would mean that for non-cosigned transactions we would use 22 bytes (1 for array length and 21 for Cosigner data) for the Sender instead of 20. But at the same time each cosigner would add just 21 byte and if the cosigner (as we currently know it) is the same as sender we would only use 22 bytes instead of 42. If we're to apply this encoding to testnet's set of transactions that would give us 269 * 22 + 1 * 21 + 2 * 21 = 5981 which is more than 30% better than post-preview2 scheme. And that's counting all failed and deploying transactions that won't be typical on a real network, so the gain there would be even closer to 50% on mainnet.

These changes are just encoding changes, it's easy to keep Sender in the code (as it's an important property of transaction) and what is currently known as Cosigners can just be a synonym of Signers. And these changes do save some space and reduce encoding/decoding overhead. But at the same time they open up a possibility for another little optimization.

GetScriptHashesForVerifying optimization

Transaction's GetScriptHashesForVerifying() implementation makes a union of Cosigners and Sender and then sorts the result to return an array of hashes to check witnesses against. There was a proposal in #1651 to remove this sorting, but it couldn't be done with the way Sender and Cosigners work now for the reasons mentioned there.

But if we're to use Signers outlined here, there would be no need to make a union operation as well as sorting, we should just specify that the order of Signers must exactly match the order of Witnesses. And it's up to transaction's creator to satisfy that condition.

Obviously, removing these operations would be beneficial for TPS, so it's not just space savings.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • P2P (TCP)
@roman-khimov roman-khimov added the Discussion Initial issue state - proposed but not yet accepted label Jul 3, 2020
@shargon
Copy link
Member

shargon commented Jul 3, 2020

Signers and Sender=> Signers[0] it's good for me

@vncoelho
Copy link
Member

vncoelho commented Jul 3, 2020

Good to me as well.

@Tommo-L
Copy link
Contributor

Tommo-L commented Jul 3, 2020

Signers and Sender=> Signers[0] it's good for me

Good for me.

If possible, could we merge Cosiginer and Witness into Signer, as a Cosinger has a witness corresponding to it, and I think witness is a property of Cosigner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants