What is the Reason For 0.4.25 Forks ?

[vikram]

I guess whether I am worried about it or not depends on how the expiration time is calculated. Can the expiration time be left unset in the transactions? Did the client at any point in time in BitShares history leave the expiration time blank? Does the client by default use some fixed offset after the time the transaction is constructed as the expiration time? Is the granularity of the expiration time 10 seconds or less?

If the old client didn't set an expiration time, then it is totally understandable why there could be multiple duplicates (in the sense that they are the same operation) that are each legitimate. But if the client always sets an expiration time and uses a fixed offset from the current time as the expiration time, I find the probability of creating a transaction with the same operations and same expiration time to be incredibly unlikely. I can understand it is not a priority since there doesn't seem to be anyone in the community claiming they had any funds stolen or unauthorized withdrawals, but it is incredibly peculiar that there were duplicates (assuming the expiration time is always set to a fixed offset of the current time) and it would be interesting to investigate further.

I would have to do some investigation to be absolutely certain, but I am fairly sure that we have always had the expiration time and it must always be set to a legitimate value.

The granularity is in seconds, and the current wallet uses 1 hour from the time of construction by default. This default was changed in the past at least twice I believe. There is an upper bound of 48 hours that the blockchain allows. There is also the "wallet_set_transaction_expiration_time" which users can use to change their wallet's default.

As you said, this is not a priority without any reports of theft, but I agree it is quite strange. I've made a note about it: https://github.com/BitShares/bitshares/issues/1142 so that I remember to take a look when I have some time.

[arhag]

We have not done a thorough analysis or inventory, but there were a number of transactions (not many--definitely less than 200 in the entire blockchain) that were duplicates under the new rules yet valid under the old rules. That is to say, they had exactly the same operations and expiration time but different signatures as you said. Almost all of such transactions were quite old; there was at least one extremely recent one but it was very small and did not look like an attack. It is difficult to say whether anything else appeared as an attack without doing a full review of the offending transactions. It is not a priority, but I can take a closer look if you think that is important.

If so, does the new code allow for such possibilities in the future (using some nonce or something) or does the new client somehow construct the transactions to not ever be identical?

In practice, nothing really changes for the end-user because subsequent identical transactions they construct will likely have different expiration dates. I do not know how those previously mentioned transactions with identical operations and expiration dates were created. There is also an additional reserved field in transactions that is currently not used, but could include a nonce if this ever becomes a problem in the future.

Thank you for your responses.

I guess whether I am worried about it or not depends on how the expiration time is calculated. Can the expiration time be left unset in the transactions? Did the client at any point in time in BitShares history leave the expiration time blank? Does the client by default use some fixed offset after the time the transaction is constructed as the expiration time? Is the granularity of the expiration time 10 seconds or less?

If the old client didn't set an expiration time, then it is totally understandable why there could be multiple duplicates (in the sense that they are the same operation) that are each legitimate. But if the client always sets an expiration time and uses a fixed offset from the current time as the expiration time, I find the probability of creating a transaction with the same operations and same expiration time to be incredibly unlikely. I can understand it is not a priority since there doesn't seem to be anyone in the community claiming they had any funds stolen or unauthorized withdrawals, but it is incredibly peculiar that there were duplicates (assuming the expiration time is always set to a fixed offset of the current time) and it would be interesting to investigate further.

However, the 3 of us that put together the upgrade all still managed to completely overlook this problem due to the stress of trying to fix the malleability security issue as fast as possible. The moral of the story is--security issue or not--we need a better process for live upgrades:

Right, that is understandable. It was a good learning experience at least, and fortunately there was no permanent damage or loss of funds. Hopefully this experience will help improve the procedures to follow for testing and quality assurance of upgrades. I'm excited for DevShares.

[xeroc]

+100% to vikram ... you are THE man .. plus the rest of the team :)

We are all just human and learn by mistakes ..

Excited to see devshares soon :)

[vikram]

If so, does the new code allow for such possibilities in the future (using some nonce or something) or does the new client somehow construct the transactions to not ever be identical?

In practice, nothing really changes for the end-user because subsequent identical transactions they construct will likely have different expiration dates. I do not know how those previously mentioned transactions with identical operations and expiration dates were created. There is also an additional reserved field in transactions that is currently not used, but could include a nonce if this ever becomes a problem in the future.

[vikram]

We applied the change to the main network and discovered that there was one transaction that was considered a duplicate under the new system that was processed by the old system.

Can you provide anymore details about that. Was that a "legitimate" duplicate in the sense that it followed identical operations as a previous transaction sent to the blockchain but was signed with different signatures? If so, does the new code allow for such possibilities in the future (using some nonce or something) or does the new client somehow construct the transactions to not ever be identical? If it wasn't "legitimate" was it a transaction that uses a non-canonical signature or alternatively stuffed a superfluous signature to create a unique transaction ID? If so, does that mean that was an intentional attack transaction?

We have not done a thorough analysis or inventory, but there were a number of transactions (not many--definitely less than 200 in the entire blockchain) that were duplicates under the new rules yet valid under the old rules. That is to say, they had exactly the same operations and expiration time but different signatures as you said. Almost all of such transactions were quite old; there was at least one extremely recent one but it was very small and did not look like an attack. It is difficult to say whether anything else appeared as an attack without doing a full review of the offending transactions. It is not a priority, but I can take a closer look if you think that is important.

Delegates started upgrading.   Duplicate transaction checking was "disabled" until the specified fork block.  Delegates started producing blocks that included duplicate transactions and thus were rejected by the non upgraded clients.   

So v0.4.25 wasn't using the old duplicate transaction checking code until block 1249400 when it would then switch to the new one? It was simply not checking for duplicate transactions AT ALL until block 1249400? And then when people submitted a transaction it appeared once to a delegate client but then lingered around and the next delegate client included the same transaction again in their block because they didn't have the code to check it was a duplicate? Am I understanding that correctly? I think I am missing something.

This description is correct. Obviously extremely stupid to do in hindsight, and does not even match how we normally do upgrades where we always keep the old logic and then synchronously switch all nodes to the new logic. However, the 3 of us that put together the upgrade all still managed to completely overlook this problem due to the stress of trying to fix the malleability security issue as fast as possible. The moral of the story is--security issue or not--we need a better process for live upgrades:

We are reviewing our options, but the conclusion is clear.   DevShares needs to come out as soon as possible because it is the most likely way we would have caught this bug.

[vikram]

The reason is bad software engineering practices. Lack of solid test framework. It's irresponsible and should be fixed yesterday.

Bytemaster and I agree that this issue demonstrates a glaring hole in our process for upgrading a live network. As he said:

We are reviewing our options, but the conclusion is clear.   DevShares needs to come out as soon as possible because it is the most likely way we would have caught this bug.

If you have other suggestions, I am happy to hear them.

[arhag]

We applied the change to the main network and discovered that there was one transaction that was considered a duplicate under the new system that was processed by the old system.

Can you provide anymore details about that. Was that a "legitimate" duplicate in the sense that it followed identical operations as a previous transaction sent to the blockchain but was signed with different signatures? If so, does the new code allow for such possibilities in the future (using some nonce or something) or does the new client somehow construct the transactions to not ever be identical? If it wasn't "legitimate" was it a transaction that uses a non-canonical signature or alternatively stuffed a superfluous signature to create a unique transaction ID? If so, does that mean that was an intentional attack transaction?

Delegates started upgrading.   Duplicate transaction checking was "disabled" until the specified fork block.  Delegates started producing blocks that included duplicate transactions and thus were rejected by the non upgraded clients.   

So v0.4.25 wasn't using the old duplicate transaction checking code until block 1249400 when it would then switch to the new one? It was simply not checking for duplicate transactions AT ALL until block 1249400? And then when people submitted a transaction it appeared once to a delegate client but then lingered around and the next delegate client included the same transaction again in their block because they didn't have the code to check it was a duplicate? Am I understanding that correctly? I think I am missing something.

[bytemaster]

Can I safely assume that 0.4.26 will stay around for some days to weeks now? I will be leaving to airport for a 15h flight ..

Yes, that better be the case. :)

[xeroc]

Can I safely assume that 0.4.26 will stay around for some days to weeks now? I will be leaving to airport for a 15h flight ..

[emski]

@Bytemaster
I wonder if you can implement the testnet into the clients.
Essentially double all the actions it does -> once for the real network and once for the test network.
If testing is paid a lot of users/delegates would be willing to enable that feature. And BTS will have testing environment much closer to the real.

[emski]

We are reviewing our options, but the conclusion is clear.   DevShares needs to come out as soon as possible because it is the most likely way we would have caught this bug.

That is what I wanted to see. Sorry if the questions I asked were a bit unpleasant or harsh. Bugs happen regardless how much you think over the issue(s).

Testing and release procedures might expose a lot of the bugs.

[bitmeat]

The reason is bad software engineering practices. Lack of solid test framework. It's irresponsible and should be fixed yesterday.

[sschechter]

Thanks for the explanation

[abit]

We implemented a change in how we check for duplicate transactions.  Tested it.  It worked.
We applied the change to the main network and discovered that there was one transaction that was considered a duplicate under the new system that was processed by the old system.
Vikram disabled duplicate checking until the scheduled fork block so that we could validate the old chain.
Vikram tested syncing up with the old chain and it worked. 
Vikram tested producing a block with the upgraded version and it worked.
Announce upgrade.

Delegates started upgrading.   Duplicate transaction checking was "disabled" until the specified fork block.  Delegates started producing blocks that included duplicate transactions and thus were rejected by the non upgraded clients.   

So the testing we needed was "dev share" test net because the bug was is the "handoff / upgrade" code that didn't reveal itself until after we had a chain with multiple delegates running the new code. 

In retrospect the issue could have been easily avoided with a little thinking, but we were rushing to get a security fix in.

We are reviewing our options, but the conclusion is clear.   DevShares needs to come out as soon as possible because it is the most likely way we would have caught this bug.

Ah this is the reason. So the 0.4.25 went wrong.
I was wondering why my balance got doubled  :P :P

[bytemaster]

We implemented a change in how we check for duplicate transactions.  Tested it.  It worked.
We applied the change to the main network and discovered that there was one transaction that was considered a duplicate under the new system that was processed by the old system.
Vikram disabled duplicate checking until the scheduled fork block so that we could validate the old chain.
Vikram tested syncing up with the old chain and it worked. 
Vikram tested producing a block with the upgraded version and it worked.
Announce upgrade.

Delegates started upgrading.   Duplicate transaction checking was "disabled" until the specified fork block.  Delegates started producing blocks that included duplicate transactions and thus were rejected by the non upgraded clients.   

So the testing we needed was "dev share" test net because the bug was is the "handoff / upgrade" code that didn't reveal itself until after we had a chain with multiple delegates running the new code. 

In retrospect the issue could have been easily avoided with a little thinking, but we were rushing to get a security fix in.

We are reviewing our options, but the conclusion is clear.   DevShares needs to come out as soon as possible because it is the most likely way we would have caught this bug.