Move context lock into network.issueTx
#2525
Conversation
joshua-kim
requested review from
abi87,
danlaine and
StephenButtolph
as code owners
joshua-kim
force-pushed
the
move-ctx-lock
branch
from
8f51db4
to
0268321
Compare
joshua-kim
force-pushed
the
move-ctx-lock
branch
3 times, most recently
from
8db7029
to
ab299db
Compare
joshua-kim
force-pushed
the
move-ctx-lock
branch
2 times, most recently
from
578fc8e
to
be0321c
Compare
| return fmt.Errorf("couldn't format address: %w", err) | ||
| } | ||
|
|
||
| return s.vm.issueTx(req.Context(), tx) |
There was a problem hiding this comment.
I was slightly concerned providing req.Context() here in the X-chain because these contexts can be cancelled by the client invoking this API... We also document that if AppSender returns an error it should be treated as FATAL... However for the rpcchainvm the gRPC server may return an error if the provided context is cancelled... We never expect the platformvm to run in to this situation... but wanted to note this so that we could talk about it
There was a problem hiding this comment.
We also document that if AppSender returns an error it should be treated as FATAL
I remember this being an invariant but I never understood why. Why can't a sender just log the error and move on? If we're letting the request context be cancelled by the client which is a legitimate use-case I feel like we shouldn't be expected to fatal.
There was a problem hiding this comment.
The comment was a prior invariant before we passed the context over the gRPC. Feel free to make a PR to remove that comment
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
joshua-kim
force-pushed
the
move-ctx-lock
branch
from
26a49ec
to
8f49622
Compare
Why this should be merged
P-chain counterpart to #2524
How this works
Moves the context lock grab into
issueTxto be held while verification is being doneHow this was tested
UTs