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.

Sign up for GitHub

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

Jump to bottom

P2P AppError handling #2248

Merged
merged 13 commits into from
Dec 12, 2023
Merged

P2P AppError handling #2248

merged 13 commits into from
Dec 12, 2023

Conversation

joshua-kim
Copy link
Contributor

@joshua-kim joshua-kim commented Nov 2, 2023 •
edited
Loading

Why this should be merged

Adds support to handle the new AppRequestFailed p2p message type. This allows VMs to define errors where previously they would just drop AppRequest messages and the requester would determine a request failed if it timed out.

How this works

Modifies the VM *AppRequestFailed interface functions to take in an error as a parameter which indicates the application-defined reason for failure.

Previously, this was only called when the network timed out. Now, timeouts are a subset of errors that may occur.

How this was tested

UT

@joshua-kim joshua-kim changed the title AppRequestFailed handling P2P AppRequestFailed handling Nov 2, 2023
@joshua-kim joshua-kim force-pushed the vm-app-error branch 2 times, most recently from af944e5 to de8c863 Compare November 2, 2023 05:25
@joshua-kim joshua-kim self-assigned this Nov 2, 2023
@joshua-kim joshua-kim added the networking This involves networking label Nov 2, 2023
@joshua-kim joshua-kim marked this pull request as ready for review November 2, 2023 18:27
danlaine
danlaine reviewed Nov 7, 2023
View reviewed changes
network/p2p/router_test.go Outdated Show resolved Hide resolved
danlaine
danlaine reviewed Nov 7, 2023
View reviewed changes
network/p2p/router_test.go Outdated Show resolved Hide resolved
@joshua-kim
Copy link
Contributor Author

Pending feedback on avalanche-foundation/ACPs#25

joshua-kim
joshua-kim commented Nov 16, 2023
View reviewed changes
network/p2p/client.go Show resolved Hide resolved
network/p2p/router_test.go Outdated Show resolved Hide resolved
snow/engine/common/engine.go Outdated Show resolved Hide resolved
@joshua-kim joshua-kim changed the title P2P AppRequestFailed handling P2P AppError handling Nov 17, 2023
abi87
abi87 reviewed Nov 19, 2023
View reviewed changes
snow/engine/common/engine.go Outdated Show resolved Hide resolved
abi87
abi87 reviewed Nov 20, 2023
View reviewed changes
snow/networking/sender/sender_test.go
Comment on lines +208 to +212
require.NoError(ctx.Err())

failedLock.Lock()
defer failedLock.Unlock()

failedVDRs.Add(nodeID)
wg.Done()
return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it a cleanup that was needed before common.AppErr or is it related to these changes?

Copy link
Contributor Author

@joshua-kim joshua-kim Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is because previously the other function signatures could use the common failed function since they all had the same signatures, since this changes the signature of AppRequestFailed we have to explicitly create one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also can't just call failed inside of here because the linter flags failed for always returning nil (not sure why it doesn't notice it if we copy-paste)....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so weird

abi87
abi87 suggested changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@abi87 abi87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits and a proposal for a slightly different error handling

@joshua-kim joshua-kim removed the request for review from marun December 6, 2023 20:02
@joshua-kim
AppError handling
aed63eb 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
89f47cc 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
28b3cec 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
0c54f1d 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
lint
2e5596c 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
tests
bb857dc 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
de6ebed 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
fix tests
3a8d3c8 
Signed-off-by: Joshua Kim <[email protected]>
StephenButtolph
StephenButtolph reviewed Dec 12, 2023
View reviewed changes
snow/networking/router/chain_router_test.go Outdated
Comment on lines 861 to 871
{
name: "AppResponse",
responseOp: message.AppResponseOp,
responseMsg: message.InboundAppResponse(ids.Empty, requestID, []byte("responseMsg"), ids.EmptyNodeID),
timeoutMsg: message.InboundAppError(ids.EmptyNodeID, ids.Empty, requestID, 123, "error"),
},
{
name: "CrossChainAppResponse",
responseOp: message.CrossChainAppResponseOp,
responseMsg: message.InternalCrossChainAppResponse(ids.EmptyNodeID, ids.Empty, ids.Empty, requestID, []byte("responseMsg")),
timeoutMsg: message.InternalCrossChainAppError(ids.EmptyNodeID, ids.Empty, ids.Empty, requestID, 123, "error"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: Could we add *AppError as the responseMsg to these test vectors? I didn't actually think this would work - but because of how we currently trigger timeouts it does work as expected.

@StephenButtolph StephenButtolph added this to the v1.10.18 milestone Dec 12, 2023
StephenButtolph
StephenButtolph approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@StephenButtolph StephenButtolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After these 3 nits this LGTM. Thanks for leading this!

message/internal_msg_builder.go Show resolved Hide resolved
message/ops.go Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph added this pull request to the merge queue Dec 12, 2023
Merged via the queue into dev with commit 4be744e Dec 12, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the vm-app-error branch December 12, 2023 20:31
@joshua-kim joshua-kim mentioned this pull request Dec 12, 2023
Merged
joshua-kim added a commit that referenced this pull request Dec 13, 2023
@joshua-kim
Squashed commit of the following:
f5f9177 
commit 82fbc97
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 12 18:30:09 2023 -0500

    Add ACP signaling (#2476)

commit ac5a00e
Author: Joshua Kim <[email protected]>
Date:   Tue Dec 12 17:42:32 2023 -0500

    Refactor p2p unit tests (#2475)

    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Dan Laine <[email protected]>

commit 0b2b109
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 12 16:48:28 2023 -0500

    `vms/platformvm`: Verify txs before building a block (#2359)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 4be744e
Author: Joshua Kim <[email protected]>
Date:   Tue Dec 12 15:08:48 2023 -0500

    P2P AppError handling (#2248)

    Signed-off-by: Joshua Kim <[email protected]>
    Co-authored-by: Stephen Buttolph <[email protected]>

commit 7963115
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 12 14:37:59 2023 -0500

    `vms/platformvm`: Add `TestBuildBlockForceAdvanceTime` test (#2472)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit dc472ec
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 12 14:37:43 2023 -0500

    `vms/platformvm`: Permit usage of the `Transactions` field in `BanffProposalBlock` (#2451)

    Co-authored-by: Stephen Buttolph <[email protected]>

Signed-off-by: Joshua Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlaine danlaine danlaine left review comments

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@abi87 abi87 abi87 approved these changes

Assignees

@joshua-kim joshua-kim

Labels
networking This involves networking
Projects
Platform Engineering Group
Archived in project
Ski Patrol
Status: In Progress 🏗
Milestone
v1.10.18
Development

Successfully merging this pull request may close these issues.
4 participants
@joshua-kim @danlaine @abi87 @StephenButtolph