zathras
|
|
December 16, 2013, 10:03:40 AM Last edit: December 16, 2013, 10:16:12 AM by zathras |
|
I rather have multiple output only be supported for Class A for now since like Zathras said there are no sequence numbers for multisigs. Once we enforce change to multisig when wallets are more feature-rich the problem should go away anyway.
I went through all addresses where I was the odd-one out and fixed most of them except two where I think I actually did it right. It comes down to two missing transactions. They are:
ecb77ee990de29745de949462e1f6e44584c310a0da12c9fbdf86dbe6ffabcfc Missing on: Masterchain and Masterchest
and
a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef Missing on: Masterchain and MyMastercoins
Could you look at these and tell me why your implementations don't have these transactions?
I'll have to look a bit further into that one (ecb77ee990de29745de949462e1f6e44584c310a0da12c9fbdf86dbe6ffabcfc) - it's detected as a simple send but fails decoding. To be continued... EDIT: Are you falling back to peek & decode on this one? I make the data address 15efTnSCG13druGmetEp1AULCEqudtCSwq and the reference address 15a4XCuWmx2cCQVf8wZK7mqdvj5uwo1vby. The data address has sequence number 51 while the reference address has sequence number 50. It should be the other way around. This should have fallen to peek & decode, but I have a feeling since the sequence numbers don't look ambiguous, my implementation is taking seqnum 50 (the ref address) and trying to decode it probably resulting in a trapped exception. Need to set this up as a dummy transaction to watch the decoding in action - I'll try and get some more time to look at this tomorrow. Thanks
|
|
|
|
Bitoy
|
|
December 16, 2013, 10:29:59 AM |
|
a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef Missing on: Masterchain and MyMastercoins
In mymastercoins I'm parsing this as a purchase offer. Since msc purchase offers are not yet allowed the transaction is marked as invalid. Invalid Transaction. Date: 11/30/2013 5:35:43 PM Remarks: Purchase Offer not found. . This transaction affects These 2 address MM=0.00006283 1Mt1tCGyJnD6SHzBgxtg6GRJLUhqQrc4ff MC=5.00006283 MM=10.55548942 15XJoDF4xCUrWX3ES9ftWq3wnGhuRsqrLk MC=5.55548942 Once it is resolved, mymastercoins and mastercoinexplorerer is synched
|
|
|
|
Tachikoma
|
|
December 16, 2013, 10:33:46 AM |
|
I think I might be to blame here, it seems I might have a regression where Purchase Offers fall back to Simple Sends on failure. Will have to check that out.
Thanks!
|
|
|
|
grazcoin
|
|
December 16, 2013, 11:04:05 AM |
|
a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef Missing on: Masterchain and MyMastercoins
In mymastercoins I'm parsing this as a purchase offer. Since msc purchase offers are not yet allowed the transaction is marked as invalid. Invalid Transaction. Date: 11/30/2013 5:35:43 PM Remarks: Purchase Offer not found. . This transaction affects These 2 address MM=0.00006283 1Mt1tCGyJnD6SHzBgxtg6GRJLUhqQrc4ff MC=5.00006283 MM=10.55548942 15XJoDF4xCUrWX3ES9ftWq3wnGhuRsqrLk MC=5.55548942 Once it is resolved, mymastercoins and mastercoinexplorerer is synched masterchain interpretation is a bitcoin payment: https://masterchain.info/btcpayment.html?tx=a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef¤cy=MSCThe reason is that the values are different: 0.00006 to 1EXoDusjGw... 0.00006 to 1Mt1tCGyJn... 0.0006 to 1MnZ4jmQ3E.. 0.00668292 to 17oDj9dvFL... I thought all values except for the change have to be the same, or else the tx is invalid for mastercoin protocol.
|
|
|
|
grazcoin
|
|
December 16, 2013, 11:15:36 AM |
|
I see no problem in accepting multiple change addresses as long as the data packets all use the same value for the output. I need to see how much of that is actually in the spec and how much was discussed in the forums. But I think multiple chance is fine now as long as the output amount is different from the Exodus reference output.
Yep, I tried to break multiple change outputs in Class A but couldn't, so agreed with the other devs that we can allow it. I'm putting a pull in today once I get past a bunch of back-to-back meetings Grazcoin, this was just for Class A. Class B still requires change to be sent back to the sender & there is no need for allowing multiple change outputs. If we allow multiple change in one format (Class A), we should allow multiple change in others as well (Class B). Otherwise, it may be confusing. Like Tachikoma said - I see no problem in accepting multiple change addresses as long as the data packets all use the same value for the output. This holds for Class A and Class B. I already mentioned before possible uses for multiple change (e.g. tx with multiple markers), so I am pro general multiple change. Do we wait for such a tx to to force us to get to a consensus or can we agree beforehand Class B enforces a rule that requires change to be returned to sender - the rule is in place because there are no sequence numbers to identify a recipient address in a Class B transaction. Class A does not enforce such a rule so multiple change outputs weren't too much of a change. If we wanted to support multiple change outputs in Class B said rule needs to be deprecated first. OK. I will sync with that rule. It would be good to add it to the spec. What about this problem? Is there any decision about arbitrary sequence numbers? My implementation takes a literal view - the wording of the spec states that in the event of packet ambiguity using sequence numbers or output amounts, then peek & decode can be used. Thus I see this transaction as valid via Peek & Decode. Looking at the consensus check it seems MyMastercoins & Mastercoin-Explorer also consider this valid. Thanks My interpretation is that "packet ambiguity" is one of the options described originally in: https://bitcointalk.org/index.php?topic=265488.msg3190175#msg3190175and with example like [84, 205, 233] it is clear that the tx is invalid. This tx contradicts the original sequence number rules - not like in the peek_and_decode case for original ambiguity case, where a plausible explanation that fits the original rules is found. If we want to simply say that every sequence can be valid (with the help of peek_and_decode) - it should be stated in the spec.
|
|
|
|
Tachikoma
|
|
December 16, 2013, 12:30:22 PM |
|
a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef Missing on: Masterchain and MyMastercoins
In mymastercoins I'm parsing this as a purchase offer. Since msc purchase offers are not yet allowed the transaction is marked as invalid. Invalid Transaction. Date: 11/30/2013 5:35:43 PM Remarks: Purchase Offer not found. . This transaction affects These 2 address MM=0.00006283 1Mt1tCGyJnD6SHzBgxtg6GRJLUhqQrc4ff MC=5.00006283 MM=10.55548942 15XJoDF4xCUrWX3ES9ftWq3wnGhuRsqrLk MC=5.55548942 Once it is resolved, mymastercoins and mastercoinexplorerer is synched masterchain interpretation is a bitcoin payment: https://masterchain.info/btcpayment.html?tx=a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef¤cy=MSCThe reason is that the values are different: 0.00006 to 1EXoDusjGw... 0.00006 to 1Mt1tCGyJn... 0.0006 to 1MnZ4jmQ3E.. 0.00668292 to 17oDj9dvFL... I thought all values except for the change have to be the same, or else the tx is invalid for mastercoin protocol. Yes, you are correct. I will fix this in my implementation. Thanks
|
|
|
|
Tachikoma
|
|
December 16, 2013, 12:41:37 PM |
|
I rather have multiple output only be supported for Class A for now since like Zathras said there are no sequence numbers for multisigs. Once we enforce change to multisig when wallets are more feature-rich the problem should go away anyway.
I went through all addresses where I was the odd-one out and fixed most of them except two where I think I actually did it right. It comes down to two missing transactions. They are:
ecb77ee990de29745de949462e1f6e44584c310a0da12c9fbdf86dbe6ffabcfc Missing on: Masterchain and Masterchest
and
a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef Missing on: Masterchain and MyMastercoins
Could you look at these and tell me why your implementations don't have these transactions?
I'll have to look a bit further into that one (ecb77ee990de29745de949462e1f6e44584c310a0da12c9fbdf86dbe6ffabcfc) - it's detected as a simple send but fails decoding. To be continued... EDIT: Are you falling back to peek & decode on this one? I make the data address 15efTnSCG13druGmetEp1AULCEqudtCSwq and the reference address 15a4XCuWmx2cCQVf8wZK7mqdvj5uwo1vby. The data address has sequence number 51 while the reference address has sequence number 50. It should be the other way around. This should have fallen to peek & decode, but I have a feeling since the sequence numbers don't look ambiguous, my implementation is taking seqnum 50 (the ref address) and trying to decode it probably resulting in a trapped exception. Need to set this up as a dummy transaction to watch the decoding in action - I'll try and get some more time to look at this tomorrow. Thanks Yeah I'm falling back on p&d on this one. D, [2013-12-16T13:41:18.347691 #3558] DEBUG -- : Found data for address 15efTnSCG13druGmetEp1AULCEqudtCSwq D, [2013-12-16T13:41:18.348294 #3558] DEBUG -- : Looking for data sequence 51 +1 == 52 D, [2013-12-16T13:41:18.349347 #3558] DEBUG -- : Sequence: 148 for 1EXoDusjGwvnjZUyKkxZ4UHEf77z6A5S4P D, [2013-12-16T13:41:18.349637 #3558] DEBUG -- : Sequence: 50 for 15a4XCuWmx2cCQVf8wZK7mqdvj5uwo1vby D, [2013-12-16T13:41:18.349896 #3558] DEBUG -- : Sequence: 51 for 15efTnSCG13druGmetEp1AULCEqudtCSwq D, [2013-12-16T13:41:18.349940 #3558] DEBUG -- : Target address not found attempting peek & decode. SimpleSend transaction from 1Q1sFqsi8S5DxV5hz6sWLamGBp9To93iG7 for 5.00000000 Mastercoin to 15a4XCuWmx2cCQVf8wZK7mqdvj5uwo1vby.
|
|
|
|
Tachikoma
|
|
December 16, 2013, 12:46:55 PM |
|
Guys I'm documenting these edge-cases mostly for my own use on Pivotal tracker. Forum posts get lost fast but action should be taken on these transactions. I use it the following way: The story name is the transaction id, the labels are the implementations that are affected and the description describes how the transactions should or shouldn't be parsed. If you want to use this system as well please let me know your account name and I will add you. Zathras, thanks for the filtered version btw. That's exactly what I needed
|
|
|
|
DGulari
Legendary
Offline
Activity: 1386
Merit: 1000
KawBet.com - Anonymous Bitcoin Casino & Sportsbook
|
|
December 16, 2013, 03:49:00 PM |
|
Zathras, thanks for the filtered version btw. That's exactly what I needed This is going to go a long way to getting this thing perfect - fast. Bonus to Zathras
|
|
|
|
zathras
|
|
December 17, 2013, 03:01:57 AM |
|
Thanks guys Quick couple of modifications to the consensus checker taking into account some feedback, probably most notable is our consensus score. Collaboratively we need this to hit 100% - round of beers on me when we do!
|
|
|
|
zathras
|
|
December 17, 2013, 03:24:39 AM |
|
Guys I'm documenting these edge-cases mostly for my own use on Pivotal tracker. Forum posts get lost fast but action should be taken on these transactions. I use it the following way: The story name is the transaction id, the labels are the implementations that are affected and the description describes how the transactions should or shouldn't be parsed. If you want to use this system as well please let me know your account name and I will add you. Zathras, thanks for the filtered version btw. That's exactly what I needed For tx a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef would that not fall to peek & decode? I'm reading things as basically seqnums should be correct & outputs should be the same, but if they're not and thus there is packet ambiguity we can then fall back to p&d. This is why Masterchest thinks it's valid. Thoughts? EDIT: There have been quite a few questions around when p&d is allowed, perhaps that's in need of some clarification in the spec?
|
|
|
|
Bitoy
|
|
December 17, 2013, 05:33:55 AM |
|
For tx a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef would that not fall to peek & decode? I'm reading things as basically seqnums should be correct & outputs should be the same, but if they're not and thus there is packet ambiguity we can then fall back to p&d. This is why Masterchest thinks it's valid. Thoughts?
EDIT: There have been quite a few questions around when p&d is allowed, perhaps that's in need of some clarification in the spec?
Mymastercoins also see a497e4fd11d2223e2129828e44b0d2110b2bb0ec3cb8a0f36bbd28f37a15ceef as a valid simple send. The way I read is "if all things failed" use "peek and decode". Valid "Peek and decode" 1. A data address is found (after removing the exodus address and the sender address from the list of output address) 2. There is one recipient address If there are more than one recipient address, 1. Get the data address sequence no. 2. Look for an address with a sequence number that is 1 less than the data address sequence no. 3. If the address is not found the transaction is invalid.
|
|
|
|
Tachikoma
|
|
December 17, 2013, 07:17:20 PM |
|
We could indeed fall back to peek and decode however right now the spec says: Has all output values equal & above the 'dust' threshold (currently 0.00005430 BTC). An additional output is permitted for the remainder of the input (the 'change' address).
I have a feeling that allowing p&d on unequal output values might increase the risk of a change address being assigned as recipient address by accident. I really want to catch human error whenever possible and support as many formats as possible but on the other hand when an error was clearly made it rather have it be invalidated then sent to the wrong address. Can anybody think of a reason that could sway the argument one way or an other?
|
|
|
|
zathras
|
|
December 17, 2013, 08:07:05 PM |
|
We could indeed fall back to peek and decode however right now the spec says: Has all output values equal & above the 'dust' threshold (currently 0.00005430 BTC). An additional output is permitted for the remainder of the input (the 'change' address).
I have a feeling that allowing p&d on unequal output values might increase the risk of a change address being assigned as recipient address by accident. I really want to catch human error whenever possible and support as many formats as possible but on the other hand when an error was clearly made it rather have it be invalidated then sent to the wrong address. Can anybody think of a reason that could sway the argument one way or an other? Good point, I've been focusing too much on the section below that (where p&d is described). JR (I assume) changed the wording when merging the appendix and made it: Ideally, all outputs should be the same (except the change). The term 'ideally' implies that it's not enforced. My appendix as submitted didn't have the word 'ideally' in it so I thought I'd forgotten something somewhere and JR had added the term to show that it wasn't an enforceable requirement. It's all starting to blur together have we had a conversation already about allowing/disallowing based on whether output amounts are the same? Thanks!
|
|
|
|
Bitoy
|
|
December 18, 2013, 06:21:29 AM Last edit: December 18, 2013, 12:41:32 PM by Bitoy |
|
Tested the following
UserA Post a sell offer 50 tmsc @ .001 each 15f7d532d988e4982ca2d9c5e99ee74810284939604adf772f59a75f7ecd9870
UserB post a purchase offer 1 tmsc @.001 waiting for payment 10c3c04b85b4d44372ce74ca8e25264626f15d5ce426487652d55e61221aa01a
UserA post a new offer 50 tmsc @ 0.1 each 5e7280c2abbbf8323ec0238a551f3da7fbf538c0a47c154b142eee6dbcd5a0aa
UserB pays UserA .0.001 ff9a479866950e7917ee98fd28291a22af9c9b7855228d3fe243fa87660a13ff
Result UserB gets 1 tmsc UserA is deducted 1 tmsc.
|
|
|
|
Tachikoma
|
|
December 18, 2013, 07:01:57 AM |
|
Have we had a conversation already about allowing/disallowing based on whether output amounts are the same?
I don't think it ever came up before. So let's make a decision on it now I think my previous arguments still stand. We need to debate whether we want to opt-in for as much valid transactions that might be interpret wrong every once in a while. Or play it safe and require some baseline requirements (same value outputs) to be met in order to reduce the risk of interpreting it wrong.
|
|
|
|
zathras
|
|
December 18, 2013, 07:09:39 AM |
|
Have we had a conversation already about allowing/disallowing based on whether output amounts are the same?
I don't think it ever came up before. So let's make a decision on it now I think my previous arguments still stand. We need to debate whether we want to opt-in for as much valid transactions that might be interpret wrong every once in a while. Or play it safe and require some baseline requirements (same value outputs) to be met in order to reduce the risk of interpreting it wrong. I'm of the view that we should button everything up as tight as possible so there can be no ambiguity as to how to decode a transaction. End users are going to be using our respective wallet software soon enough, I'm actually for as smaller 'net' as possible for transaction validity, but I do accept we need to honor transactions sent to date.
|
|
|
|
Tachikoma
|
|
December 18, 2013, 07:14:16 AM |
|
Perhaps it's time to summon the magical being known as J.R. and let him make a decision on whether to p&r transactions that have unequal output amounts.
|
|
|
|
zathras
|
|
December 18, 2013, 07:16:50 AM |
|
Perhaps it's time to summon the magical being known as J.R. and let him make a decision on whether to p&r transactions that have unequal output amounts.
<begins chanting>...
|
|
|
|
aTriz
|
|
December 19, 2013, 12:42:36 AM |
|
Perhaps it's time to summon the magical being known as J.R. and let him make a decision on whether to p&r transactions that have unequal output amounts.
<aTriz beats on his drums!>....
|
|
|
|
|