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

Class A P&D clarification #29

Closed
maran opened this issue Jan 8, 2014 · 9 comments
Closed

Class A P&D clarification #29

maran opened this issue Jan 8, 2014 · 9 comments

Comments

@maran
Copy link
Contributor

maran commented Jan 8, 2014

In order to make it easier to parse "Class A" transactions Zathras suggested we use P&D by default and make it clear in which order to decode these type of transactions. The following is my suggestion, after comments I can merge edit this into the original spec.

One of the main questions I have at this point is whether to give precedence to Level 3 over Level 2. Level 2 assumes a typo on the output value which is a very common error. See this transaction for instance. On the other hand if there are three exact output values with the same value as Exodus then it should be safe to assume that the owner of said transaction meant the third one to be the recipient address. Please let me know your opinions whether we should do Level 3 or Level 2 first after Level 1.

Decoding "Class A" transactions
In order to support as much Class A transactions as possible we will use a method called "Peek & Decode" (P&D) as standard decoding method. P&D decoding should happen as follows.

Level 1
Take all the outputs that have the same value as the value to the Exodus address. The first thing we want to do is locate the data address. In order to do this decode each address and look for the following byte checks:

  • Bytes two to eight must equal 00
  • Byte nine must equal 01 or 02

If one of these addresses is found get the sequence number of this address and add one to it. Go through the other Exodus equal output values and look for this sequence number. If the sequence number is found this is the recipient address.

Level 2
If the sequence number can't be found you can expand the searching range to include all outputs, not just the ones that have an equal Exodus amount. Repeat the steps from Level 1 in according with the outputs and see if a valid sequence number can be found this way.

Level 3
If a valid sequence number still can't be found we can try finding the correct address by ruling out the other addresses. This method is only valid if all output values are equal in size and if there are three outputs of these type of outputs total. This method is simple deduction. Collect all outputs and remove the data address and the Exodus output. The remaining output is the recipient address.

Notes: If multiple valid looking data addresses are found the transaction gets invalidated.

@ghost
Copy link

ghost commented Jan 8, 2014

Related question: Is there a preferred method of parsing the blockchain to look for these sequence numbers? The naive implementation would probably be quite slow, so I was hoping someone had figured out a fast parser so I wouldn't lose sleep over it.

@maran
Copy link
Contributor Author

maran commented Jan 8, 2014

I think that question does not belong here. The answer is too specific to each implementation anyway. I just retrieve the tx and the according outputs directly via a query to bitcoin-ruby.

@grazcoin
Copy link

grazcoin commented Jan 8, 2014

Thank you for the pull request. A clarification is indeed essential.

Level 1 should be according to the original set of rules
https://bitcointalk.org/index.php?topic=265488.msg3190175#msg3190175

"All protocol transactions should have the same output amount. If one output is different, that is the change address. If all outputs are the same, then look at sequence numbers: If there is a broken sequence (i.e. 3,4,8), then the odd-man-out is the change address (8 in this example)"

This means that first we check the sequence numbers, and only then we verify that the data address complies with the byte checks (and not the other way around).

Maybe in practice this makes no difference, but I rather stay with the original way and not reorder the steps for no reason (especially since this code is already running few months on my setup).

As for Level 2 - the original set of rules started with "All protocol transactions should have the same output amount", so I prefer considering the wrong amount typos only as last resort. My suggestion is trying P&D using outputs with equal values as Level 2.
Remark: in the near future tx will be done only using wallets, so no typos are expected. On the other hand, attacks on consensus are expected, so limiting the possibilities of non standard tx is desired.

If Level 2 doesn't work as well, we could try to rule out a typo in outputs amount (mainly for this single tx in question), and look there for valid sequence numbers. For the same reason of limiting the possibilities of non standard tx, I suggest a cleaner solution: Let's ask 15XJoDF4xCUrWX3ES9ftWq3wnGhuRsqrLk to send again 5 MSC to 1Mt1tCGyJnD6SHzBgxtg6GRJLUhqQrc4ff - this time without the typo, and invalidate the original typo tx.
I verified that such an act would not change the history, and it has no effect on other tx and balances.
From that moment, we don't allow such typos.

Another related issue which should be covered here is regarding the recipient discovery:
https://bitcointalk.org/index.php?topic=292628.msg4106402#msg4106402
which boils down to allowing a user to send coins to himself.

@maran
Copy link
Contributor Author

maran commented Jan 9, 2014

I already employ this order and judging by the consensus rate it's doing ok.

The reason Zathras and I discussed this approach is to make everything simpler. Right now the spec for Class A is pretty hard to read and the whole idea of this patch would be to remove all the side-notes and have one clear way of parsing these transactions.

I realise that it changes a lot of the logic; but that's intended. I will see if I can setup a second database that does a full reparse since Exodus and run it next to my current database to make sure this form of parsing holds up. But I expect that all transactions should be the same; just easier to parse :)

grazcoin added a commit to grazcoin/mastercoin-tools that referenced this issue Jan 10, 2014
@ghost
Copy link

ghost commented Jan 12, 2014

I think some sort of example would be very helpful for newbs trying to implement the spec. Kind of how it is shown for Class B tx:

decoded data address (in bytes): 
[ 0, 234, 0, 0, 0, 0, 0, 0, 0,  1,  0, 0, 0, 0, 38, 162, 10, 246,  0, 0, 0, 77, 202, 30, 189 ]
  0   1   2  3  4  5  6  7  8   9  10  11 12 13  14  15  16  17  18  19  20  21  22  23  24
     seq  --------empty------ curr

To make things more obvious. I can make a PR with something like this if it seems helpful.

Also, a question (sorry if it comes off as newbish) but how can a Level 3 tx be valid if there is no found sequence #? Otherwise, I prefer Level 2 after Level 1, since it makes sense to group the decode methods that use the sequence number together.

@maran
Copy link
Contributor Author

maran commented Jan 13, 2014

We are most likely going for an other direction on this ticket. Zathras has some suggested an other way of wording things that leaves the actual implementation to the implementor instead of drawing it all out.

Level 3 can still be considered valid because you can rule out all other options. Let's ignore the sequences on this transaction but just look at it. Now we know by P&D that the data address is 1KsWsi7jHXte1sNc2bMQCpV2xY42MBi7Rj. We know the recipient address is not the Exodus address (no serious payment would do that). That leaves 1L2RvHFHoviCzk2CVJWhyKKYh8kzo3sbnq. There is some risk invalid once you get to level 3; but if you wanted to be sure your transaction would have been proper you should have taken more care crafting it or used Class B instead! ;)

I would advise you to look at other implementations if you want to see how they are parsing it. I made my library very verbose while checking Class A; for instance:

bin/mastercoin_transaction lookup ead9319d7ef35894996e6a246f20922abe3d9c44ba08d55217f452e02a3b433a
D, [2014-01-13T10:28:37.149302 #60546] DEBUG -- : Found data for address 1KsWsi7jHXte1sNc2bMQCpV2xY42MBi7Rj
D, [2014-01-13T10:28:37.150101 #60546] DEBUG -- : Looking for data sequence 207 +1 == 208
D, [2014-01-13T10:28:37.170174 #60546] DEBUG -- : Sequence: 148 for 1EXoDusjGwvnjZUyKkxZ4UHEf77z6A5S4P
D, [2014-01-13T10:28:37.170546 #60546] DEBUG -- : Sequence: 208 for 1L2RvHFHoviCzk2CVJWhyKKYh8kzo3sbnq
D, [2014-01-13T10:28:37.170612 #60546] DEBUG -- : Target address found 1L2RvHFHoviCzk2CVJWhyKKYh8kzo3sbnq
D, [2014-01-13T10:28:37.170860 #60546] DEBUG -- : Sequence: 207 for 1KsWsi7jHXte1sNc2bMQCpV2xY42MBi7Rj
SimpleSend transaction from 1MaStErt4XsYHPwfrN9TpgdURLhHTdMenH for 15.00000000 Mastercoin to 1L2RvHFHoviCzk2CVJWhyKKYh8kzo3sbnq.

Hope this helps.

@ghost
Copy link

ghost commented Jan 13, 2014

I'm still confused as to why the preference is to leave ambiguity in the spec rather than eliminate it. For Class
B transactions there is a clear diagram and example case showing what to expect for each step of the multisig transaction. Since it is intended others will implement the spec (maybe devs on MtGox for example) I think the best course of action is to eliminate ambigous wording.

When I was researching how to decode Class A tx, the only note provided how the sequence number is obtained is a footnote, but then it's more clearly explained in Class B. I think this is hindering backwards-compatibility and will cause a number of implementations to show incorrect values for Class A tx in the future.

It is 'ok' if you say 'we prefer it ambiguous, so that they might check implementations for exact details', so that I might understand how you guys are thinking about the spec's clarity.

@ghost
Copy link

ghost commented Jan 13, 2014

Thanks for explaining the Level 3 decode. It makes more sense now. I think I'll look at the implementation for any further questions I might have. :)

@maran
Copy link
Contributor Author

maran commented Jan 14, 2014

Closing this in favor of #36

@maran maran closed this as completed Jan 14, 2014
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

No branches or pull requests

2 participants