REST API: return participation key from generate endpoint #6158
Conversation
There was a problem hiding this comment.
I'm unsure about the idea of blocking until the part keys are created. Generation can take a very long time because of Falcon keys.
Perhaps we can generate and return the ID immediately, and the caller can be expected to ask for the details, perhaps repeatedly, until they have been generated.
daemon/algod/api/algod.oas2.json
Outdated
| @@ -5629,4 +5629,4 @@ | |||
| "name": "private" | |||
| } | |||
| ] | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Let's not remove end-of-file newlines gratuitously, else we'll end up with commit wars that bring them back and remove them depending on who edits them last.
| // Semaphore was acquired, generate the key. | ||
| go func() { | ||
| defer v2.KeygenLimiter.Release(1) | ||
| err := v2.generateKeyHandler(address, params) | ||
| if err != nil { | ||
| v2.Log.Warnf("Error generating participation keys: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
This starts a (nearly empty) go routine which immediately releases the KeygenLimiter semaphore. Therefore, this allows any number of keygens to happen concurrently. I doubt that's what we want, so the defer line should be moved outside of the go routine, which should go away. I would put it before line 307.
Since generating part keys can take a very long time, I'm not sure what will happen to the TCP connection that's being held open the entire time. I expect timeouts will occur in many situations.
There was a problem hiding this comment.
This was the only was I could get the lock to release while testing. I will take another look at it.
There was a problem hiding this comment.
Somewhat off-topic. It's unclear to me why a semaphore.Weighted was used. It seems to be used like a simple sync.WaitGroup. You don't have to fix that, I suppose.
@jannotti This is a good question, I am currently watching the participation endpoint during key generation. I agree, returning the participation ID would be an improvement to the current implementation. It would still require the caller to handle errors, effectively just shifting the problem to a different machine and adds RPC overhead to the node. Since it's a private api guarded by the admin token, it may make sense to allow a long lived connection. At least that was my thinking, would love to know what you have in mind. |
Co-authored-by: John Jannotti <[email protected]>
I think that's fine, in principle. But I think that extremely long-lived TCP connections that don't send any data are sometimes subject to timeouts. For example, if there is a NAT box between client and Polling by the client using an ID that is returned quickly may seem inelegant, but I think it's the most robust. A particularly annoying thing that could occur:
|
I agree with the returning just an ID approach as long as we can handle the errors elegantly it should be a good solution. I wasn't able to find an elegant way to handle the error when keys already existed for the given
Absolutely, whatever you think is best. The server generally should be in charge of state and in this case it would require a state machine on clients for a resource owned by the service. We could help the consumers here, maybe a What do you think @jannotti? |
|
I'm not seeing a way to return the ID since it is dependent on the generated secrets (I may be missing something obvious). There are quite a few error messages that are not returned, is this something that is expected? Just refactoring the error handling a bit may get us what we need until we find a solution for the ID |
That seems to be the case, which explains why this wasn't done before, despite the comment saying that maybe it could be done. Without looking into at all, I don't know why the ID is a hash of the contents. I don't know if that's just "cute" - a interesting way to generate a unique ID - or an actual meaningful aspect of the implementation. Generally speaking, I like my database IDs to just be sequential, or perhaps uuids. I'd support such a change if it's staightforward. If we did that, we could generate the ID first, and perform the insertion later, with the pregenerated ID. I don't know if there's any code that needs to find the right partkey based on contents. That would explain the deterministic ID. If there is, it would have to be refactored - presumably it could be done with a more elaborate |
returns participation id on generate
|
It appears as if the table has a sequential primary key and is storing the Participation ID as an additional field. I was able to remove the generated keys from the Participation Identity which produces the hash. This should keep changes low but you raise a good point
It seems like it could be the former, I will ask around to see if anyone has any information. If this implementation is accepted it would be good to expose a few errors before shipping (just the most common ones) |
|
I've come up empty for justifications so far. The ID was introduced with the RPC endpoints in #3164. I assume it was just an ad-hoc decision. |
Summary
model.ParticipationKeyfrom/v2/participation/generateTest Plan