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

Add ACP signaling #2476

Merged
merged 7 commits into from Dec 12, 2023
Merged

Add ACP signaling #2476

merged 7 commits into from Dec 12, 2023

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

This enables node operators to signal their opinions of proposed ACPs without requiring additional software.

This is purely for signaling, and does not result in activation of any sufficiently supported ACPs.

How this works

Adds --acp-support config.
Adds --acp-object config.
Adds SupportedACPs and ObjectedACPs to the Version message.
Adds SupportedACPs and ObjectedACPs to the peer.Info API call output.
Adds peer.acps API call.

How this was tested

@StephenButtolph StephenButtolph added this to the v1.10.18 milestone Dec 12, 2023
@StephenButtolph StephenButtolph self-assigned this Dec 12, 2023
patrick-ogrady
patrick-ogrady reviewed Dec 12, 2023
View reviewed changes
node/node.go Outdated Show resolved Hide resolved
@StephenButtolph StephenButtolph added the networking This involves networking label Dec 12, 2023
patrick-ogrady
patrick-ogrady reviewed Dec 12, 2023
View reviewed changes
config/flags.go
@@ -92,6 +92,10 @@ func addNodeFlags(fs *pflag.FlagSet) {
// Network ID
fs.String(NetworkNameKey, constants.MainnetName, "Network ID this node will connect to")

// ACP flagging
fs.IntSlice(ACPSupportKey, nil, "ACPs to support adoption")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use UintSlice here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Viper doesn't seem to have support for UintSlice - so I felt it was more straight-forward to use IntSlice in both places.

patrick-ogrady
patrick-ogrady reviewed Dec 12, 2023
View reviewed changes
node/node.go
@@ -439,6 +439,25 @@ func (n *Node) initNetworking() error {
)
}

// We allow nodes to gossip unknown ACPs in case the current ACPs constant
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this check/log should be done in config/config. Found it "odd" that we checked some conditions there and some here 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

But not sure if there is some pattern we are following of what we verify here vs there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's here because we have a ref to the logger here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As dan noted - this is done here because the logger is initialized. We aren't really "verifying" here... because we do still allow the node to include acps that haven't been included in the client yet

danlaine
danlaine approved these changes Dec 12, 2023
View reviewed changes
message/outbound_msg_builder.go
@@ -30,6 +30,8 @@ type OutboundMsgBuilder interface {
myVersionTime uint64,
sig []byte,
trackedSubnets []ids.ID,
supportedACPs []uint32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not specific to this PR, but we might consider renaming Version to Handshake or something to reflect the fact that it now contains a lot more than just the version of the sender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator

Choose a reason for hiding this comment

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

#2479

ask and ye shall receive

joshua-kim
joshua-kim reviewed Dec 12, 2023
View reviewed changes
api/info/service.go Show resolved Hide resolved
message/outbound_msg_builder.go
@@ -30,6 +30,8 @@ type OutboundMsgBuilder interface {
myVersionTime uint64,
sig []byte,
trackedSubnets []ids.ID,
supportedACPs []uint32,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@danlaine danlaine mentioned this pull request Dec 12, 2023
Merged
@StephenButtolph StephenButtolph added this pull request to the merge queue Dec 12, 2023
Merged via the queue into dev with commit 82fbc97 Dec 12, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the add-acp-signaling branch December 12, 2023 23:53
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 <stephen@avalabs.org>
Date:   Tue Dec 12 18:30:09 2023 -0500

    Add ACP signaling (#2476)

commit ac5a00e
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Tue Dec 12 17:42:32 2023 -0500

    Refactor p2p unit tests (#2475)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Co-authored-by: Dan Laine <daniel.laine@avalabs.org>

commit 0b2b109
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Tue Dec 12 16:48:28 2023 -0500

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

    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 4be744e
Author: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Date:   Tue Dec 12 15:08:48 2023 -0500

    P2P AppError handling (#2248)

    Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit 7963115
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Date:   Tue Dec 12 14:37:59 2023 -0500

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

    Co-authored-by: Stephen Buttolph <stephen@avalabs.org>

commit dc472ec
Author: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
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 <stephen@avalabs.org>

Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
@dhrubabasu dhrubabasu mentioned this pull request Mar 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlaine danlaine danlaine approved these changes

@joshua-kim joshua-kim joshua-kim left review comments

@patrick-ogrady patrick-ogrady patrick-ogrady approved these changes

@gyuho gyuho Awaiting requested review from gyuho gyuho is a code owner

@hexfusion hexfusion Awaiting requested review from hexfusion

Assignees

@StephenButtolph StephenButtolph

Labels
networking This involves networking
Projects
Platform Engineering Group
Archived in project
Milestone
v1.10.18
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@StephenButtolph @patrick-ogrady @danlaine @joshua-kim