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

Aggegate bonded - Failure_Core_Future_Deadline #1490

Closed
cryptoBeliever opened this issue May 12, 2021 · 14 comments · Fixed by #1713
Closed

Aggegate bonded - Failure_Core_Future_Deadline #1490

cryptoBeliever opened this issue May 12, 2021 · 14 comments · Fixed by #1713
Assignees
Labels
P2 Issue Feature or UI issue preventing an action, Incomplete process or crashing during action Sprint 4

Comments

@cryptoBeliever
Copy link
Contributor

Steps:

  1. Change system clock +1h
  2. Try to send aggregate bonded transaction (for example multisig)

Result: Announcing hash lock failes with error "Failure_Core_Future_Deadline":

image

Questions:

  1. Why Hash lock transaction deadline is set to 6 hours? Hash lock is confirmed immediately. Why it has 6h instead of 2h like a normal transaction?

  2. When the user operating system clock is ahead 1minute (or node clock 1minute behind) the user gets an error as above. I assume here that the max deadline for the hash lock is 6h but it seems correct assumption.

  3. Wouldn't better to use node time (/node/time) instead of local user time?

@cryptoBeliever cryptoBeliever added the P2 Issue Feature or UI issue preventing an action, Incomplete process or crashing during action label May 12, 2021
@bassemmagdy bassemmagdy self-assigned this May 18, 2021
@fboucquez
Copy link
Contributor

fboucquez commented May 31, 2021

Some notes:

  1. 6 hours hash lock deadline is too long and top limit, the user is not going to wait for 6 hours before the wallet announces the bonded transaction.
    https://github.com/nemgrouplimited/symbol-desktop-wallet/blob/96b1678b5943b631299511d165d074f4a1219e35/src/services/TransactionCommand.ts#L173
    It could be the default 2 hours or even less.

  2. 48 hours bonded is the top limit, the network may reject when the local time is slightly unsynchronized.
    https://github.com/nemgrouplimited/symbol-desktop-wallet/blob/96b1678b5943b631299511d165d074f4a1219e35/src/services/TransactionCommand.ts#L162
    Both 6 and 48 hours should be wallet configuration, not hardcoded.

  3. I've created a service in the SDK that offsets any error. The wallet could create and share the service each time the repo factory is created via stores.
    feat: DeadlineService symbol-sdk-typescript-javascript#790
    The wallet could use DeadlineService.createDeadlineUsingOffset.

@coiki
Copy link

coiki commented Jun 9, 2021

@surekabpm seems like this task was automatically moved to the Issue Backlog after you reopened it. Can you confirm it needs to be reopened?

@surekabpm
Copy link

This one by mistake I closed it and later have to reopen

@cryptoBeliever
Copy link
Contributor Author

This should be tested after code review.

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Jun 30, 2021

@bassemmagdy announcing transaction doesn't work:

image

@bassemmagdy
Copy link
Contributor

@cryptoBeliever please provide some steps to reproduce.

@postoronnii
Copy link

@bassemmagdy Create multisig account with 2 co-signors and initiate transaction from multisig account.

@cryptoBeliever
Copy link
Contributor Author

Previous error I'm not able reproduce but now I can see errors with Timestamp too far in the future after a few tries of sending:

https://share.getcloudapp.com/z8uOGz6m

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Jul 4, 2021

Still got occasionally errors like this (but hard to reproduce exact steps):

image

Also, sdk is not released (alpha version used) so my suggestion is not to include in the next wallet release.

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Sep 21, 2021

@bassemmagdy looks like the number of API calls to the /time endpoint is too big. Please check video:
https://share.getcloudapp.com/Apu9WrYr

Maybe wallet should have only time synchronization periodically called like once per minute?

@cryptoBeliever
Copy link
Contributor Author

As we discussed on the daily meeting there will be a change regarding time synchronization with a node. To avoid frequently ask /time endpoint.

@cryptoBeliever
Copy link
Contributor Author

cryptoBeliever commented Sep 23, 2021

https://github.com/symbol/symbol-sdk-typescript-javascript/blob/81f5397b6f2db1d1c3eea15d3e8a41b6b82ae31f/src/service/DeadlineService.ts#L64

localTimeOffset and createDeadlineUsingOffset

Allow to calculate "server" deadlines without calling /time endpoint each time.

We should probably also refresh server time each X minutes (like 5) and when user switch node.

@cryptoBeliever
Copy link
Contributor Author

All issues fixed 👍

@cryptoBeliever
Copy link
Contributor Author

@bassemmagdy please merge into dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Issue Feature or UI issue preventing an action, Incomplete process or crashing during action Sprint 4
Projects
None yet
6 participants