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

Robustness: concurrency issues #345

Closed
PascalinDe opened this issue Sep 6, 2023 · 9 comments
Closed

Robustness: concurrency issues #345

PascalinDe opened this issue Sep 6, 2023 · 9 comments

Comments

@PascalinDe
Copy link
Contributor

When transactions come in too fast to DELA, they seem to get invalid nonces:

d-voting-dela-worker-0-1  | 2023-09-06T09:44:36Z INF pkg/mod/go.dedis.ch/dela@v0.0.0-20221010131641-9c479e68be18/core/ordering/cosipbft/mod.go:387 > block event addr=dela-worker-0:2000 index=43 root=c32b3060
d-voting-dela-worker-0-1  | 2023-09-06T09:44:36Z ERR d-voting/proxy/election.go:208 > failed to submit txn error="failed to add transaction to the pool: store failed: invalid transaction: unacceptable transaction: nonce '43' < '44'" role=evoting-proxy

which leads to them just getting "ignored" i.e. votes are not taken into account, nodes are not getting initialised, ...

@ineiti
Copy link
Member

ineiti commented Sep 6, 2023

it might in fact be quite difficult for the authentication server (AS) to handle that case! What I think happens is:

  1. first request comes in
  2. the AS asks Dela for the current counter
  3. AS sends a TX1 to Dela
  4. second request comes in
  5. AS asks Dela for current counter, which is not increased, as TX1 hasn't been handled by Dela yet
  6. AS sends TX2 to Dela with the same counter
  7. Dela only accepts one of the two TX

So the AS would have to store the counter in its database and increase it for every TX it sends.

@pierluca
Copy link
Contributor

pierluca commented Sep 6, 2023

@ineiti nope, you're missing the proxy that does the "transaction manager".
Luckily, everything is co-located (for now) within the Dela binary.

@ineiti
Copy link
Member

ineiti commented Sep 7, 2023

Who is handling the nonce? The proxy?

@ineiti
Copy link
Member

ineiti commented Sep 7, 2023

Can you elaborate the nope? Which part of my answer?

Also copying form slack:

Pierluca Borsò-Tan: I think we might be able to store a counter on the proxy side, but we can also probably tune the parameter defining how far ahead the nonces are allowed. I can’t recall the exact name, but there’s a parameter that indicates how “early” a nonce can show up (i.e., nonces are monotonically increasing, and if we’re at nonce 10, do we tolerate 13, 15, 17 yet or not?)
linus.gasser: But if the TXs are with nonce 10, 12, and then a nonce with 11 comes in, does that also count? Or will it just accept 13+ after 12?
Pierluca Borsò-Tan: https://github.com/dedis/dela/blob/master/core/validation/simple/simple.go#L70
The answer's in this logic 🙂
linus.gasser: And once it's accepted, GetNonce will return the new nonce? So in my example, 11 will be refused?

@ineiti
Copy link
Member

ineiti commented Sep 7, 2023

Answering my question about whether nonce 11 is refused or not: looking at the logs Carine posted above, it seems that yes, it is refused once nonce 12 is accepted. So we definitely want to add a counter in the authentication server to take care of this problem.

Btw: I think MaxSequenceDifference should be exactly 0. If you're accepting holes in the nonces, you're opening a new can of problems to solve other problems. So I prefer having only one set of problems :) #ChangeMyMind

@pierluca
Copy link
Contributor

pierluca commented Sep 7, 2023

The transaction is created here and the associated nonce is managed in the same place:
https://github.com/dedis/dela/blob/master/core/txn/signed/signed.go#L301

That function is called by the transaction manager in the D-Voting proxy
https://github.com/dedis/d-voting/blob/main/proxy/txnmanager/transaction.go#L315

@nkcr
Copy link
Contributor

nkcr commented Sep 7, 2023

In my opinion you should not make the authentication server responsible for managing the nonce:

  • It should only be responsible for the authentication/authorization part, no more, no less
  • It won't be as simple as keeping a counter
  • I don't think Dela exposes an API to get the current nonce, which you'll will need in case you are out-of-sync with your counter (which will happen)

For me this should be solved at the GUI level. Every action that implies a transaction on the blockchain must be assumed to be a promise that can be resolved or rejected, and the GUI should clearly display that. When an error occurs (for example the transaction didn't get included because of the nonce) the user should be notified and have the opportunity to retry. Most of the work is about having a better error handling + feedback provided to the users (typically when I cast a ballot, I should have a confirmation that the transaction has been included, or an error explaining what happened and the opportunity to re-cast my vote)

@ineiti
Copy link
Member

ineiti commented Sep 8, 2023

For me this should be solved at the GUI level.

Some thoughts:

  • the GUI cannot know which nonce to use, so it needs to get this from the authentication server (AS)
  • the GUI doesn't know who else is trying to cast a vote, only the AS knows that
  • if I understand correctly, the AS does the signing, so the AS should also handle the nonce
  • users shouldn't see "your vote couldn't be taken into account, please try again later" when it's possible to avoid this error message

So I'm still convinced that handling the nonce at the AS level is the way to go. Of course it is not ideal to have one more job for the AS. But I'm trying to have something working for the end of the month.

The ideal way would be of course that the AS only does the authentication, and the GUI does all the rest. Unfortunately this doesn't seem to be possible without rewriting big parts of the d-voting project. But if we're re-writing that much, we'd have to consider using the old e-voting, which has a lot of functionality needed already built-in and is well stress-tested.

How did this work for the demo for Unicore? You didn't go above 256 blocks, and nobody voted concurrently?

@ineiti
Copy link
Member

ineiti commented Mar 7, 2024

Changed the way nonces are handled in the proxy where the transactions are signed. Now we can have transactions with increasing nonces in blocks. In 2024/03 we get about 8 transactions/block.

@ineiti ineiti closed this as completed Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants