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

vms/platformvm: Add decisionTxs parameter to NewBanffProposalBlock #2411

Merged
merged 28 commits into from
Dec 5, 2023
Merged

vms/platformvm: Add decisionTxs parameter to NewBanffProposalBlock #2411

merged 28 commits into from
Dec 5, 2023

Conversation

dhrubabasu
Copy link
Contributor

Why this should be merged

Preparatory work for supporting decision txs in proposal blocks

How this works

Modifies the function signature. The block type already supports the Transactions in the codec so no serializations will change. Additionally, the block verifier already checks that the length of this field is 0: https://github.com/ava-labs/avalanchego/blob/dev/vms/platformvm/block/executor/verifier.go#L54-L56

How this was tested

CI

@dhrubabasu dhrubabasu added this to the v1.10.18 milestone Dec 4, 2023
@dhrubabasu dhrubabasu self-assigned this Dec 4, 2023
danlaine
danlaine suggested changes Dec 4, 2023
View reviewed changes
vms/platformvm/block/proposal_block_test.go Outdated Show resolved Hide resolved
vms/platformvm/block/parse_test.go Show resolved Hide resolved
vms/platformvm/block/builder/builder.go Outdated Show resolved Hide resolved
@dhrubabasu dhrubabasu mentioned this pull request Dec 4, 2023
Merged
@dhrubabasu dhrubabasu changed the base branch from dev to initialize-banff-decision-txs December 4, 2023 18:24
Base automatically changed from initialize-banff-decision-txs to dev December 4, 2023 22:54
@dhrubabasu dhrubabasu changed the base branch from dev to p-chain-block-test-cleanups December 4, 2023 22:56
danlaine
danlaine reviewed Dec 5, 2023
View reviewed changes
vms/platformvm/block/parse_test.go
@@ -130,6 +133,33 @@ func TestProposalBlocks(t *testing.T) {

// backward compatibility check
require.Equal(parsedApricotProposalBlk.Txs(), parsedBanffProposalBlk.Txs())

// check that banff proposal block with decisionTxs can be built and parsed
banffProposalBlkWithDecisionTxs, err := NewBanffProposalBlock(
Copy link

Choose a reason for hiding this comment

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

Could we put decisionTxs in banffProposalBlk so that we don't need to build a new block here?

Copy link
Contributor Author

@dhrubabasu dhrubabasu Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted to retain the backward compatibility check on line 135

// backward compatibility check
require.Equal(parsedApricotProposalBlk.Txs(), parsedBanffProposalBlk.Txs())

danlaine
danlaine reviewed Dec 5, 2023
View reviewed changes
vms/platformvm/block/proposal_block_test.go Outdated
}
}

func TestNewBanffProposalBlockWithDecisionTxs(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

Do we need a new test? Why not just put decisionTxs in blk in TestNewBanffProposalBlock?

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 put a new test since BanffProposalBlocks with no decisionTxs should be valid- not sure if this is the right way to do that but open to suggestions

Copy link

Choose a reason for hiding this comment

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

Personally I feel like it'd make sense to combine these into one test, but not a blocker from me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, good suggestion 🫡

@dhrubabasu dhrubabasu requested a review from danlaine December 5, 2023 15:11
Base automatically changed from p-chain-block-test-cleanups to dev December 5, 2023 16:33
@StephenButtolph StephenButtolph added this pull request to the merge queue Dec 5, 2023
Merged via the queue into dev with commit 004a23e Dec 5, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the start-supporting-decisiontxs-in-proposal-blks branch December 5, 2023 18:19
joshua-kim added a commit that referenced this pull request Dec 6, 2023
@joshua-kim
Squashed commit of the following:
8866972 
commit b36416d
Author: Alberto Benegiamo <[email protected]>
Date:   Wed Dec 6 12:12:11 2023 -0700

    Drop Pending Stakers 1 - introduced ScheduledStaker txs (#2323)

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

commit 7df1f3a
Author: Stephen Buttolph <[email protected]>
Date:   Wed Dec 6 13:46:56 2023 -0500

    Restrict GOPROXY (#2434)

commit 21b7ab8
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 5 19:43:00 2023 -0500

    Fix platformvm.SetPreference (#2429)

commit ada692a
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 5 17:40:03 2023 -0500

    Update minimum golang version to v1.20.12 (#2427)

commit 004a23e
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 5 12:56:25 2023 -0500

    `vms/platformvm`: Add `decisionTxs` parameter to `NewBanffProposalBlock` (#2411)

    Co-authored-by: Dan Laine <[email protected]>

commit 439dc1e
Author: Alberto Benegiamo <[email protected]>
Date:   Tue Dec 5 10:46:13 2023 -0700

    ProposerVM Extend windows 0 - Cleanup (#2404)

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

commit 477157d
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 5 11:10:56 2023 -0500

    `vms/platformvm`: Cleanup some block tests (#2422)

    Co-authored-by: Dan Laine <[email protected]>

commit b6700c9
Author: Stephen Buttolph <[email protected]>
Date:   Mon Dec 4 22:24:34 2023 -0500

    Update slices dependency to use Compare (#2424)

    Co-authored-by: James Walker <[email protected]>

commit 5d9e482
Author: Dan Laine <[email protected]>
Date:   Mon Dec 4 19:27:29 2023 -0500

    allow user of `build_fuzz.sh` to specify a directory to fuzz in (#2414)

commit 2e32281
Author: Stephen Buttolph <[email protected]>
Date:   Mon Dec 4 18:30:41 2023 -0500

    Fix duplicated bootstrapper engine termination (#2334)

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

commit b741c19
Author: Dhruba Basu <[email protected]>
Date:   Mon Dec 4 17:49:15 2023 -0500

    `vms/platformvm`: Move `VerifyUniqueInputs` from `verifier` to `backend` (#2410)

commit 6aa20fc
Author: Dhruba Basu <[email protected]>
Date:   Mon Dec 4 17:32:07 2023 -0500

    `vms/platformvm`: Initialize txs in `Transactions` field for `BanffProposalBlock` (#2419)

commit c11accd
Author: Alberto Benegiamo <[email protected]>
Date:   Mon Dec 4 09:16:01 2023 -0700

    Drop Pending Stakers 0 - De-duplicate staking tx verification (#2335)

commit 05ce366
Author: marun <[email protected]>
Date:   Sat Dec 2 13:33:18 2023 -0800

    testing: Update to latest version of ginkgo (#2390)

commit 04af33e
Author: Dhruba Basu <[email protected]>
Date:   Sat Dec 2 16:33:07 2023 -0500

    `vms/platformvm`: Cleanup block builder tests (#2406)

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

commit 7623ffd
Author: Stephen Buttolph <[email protected]>
Date:   Fri Dec 1 19:39:15 2023 -0500

    Update versions for v1.10.17 (#2394)

commit be1a2ad
Author: aaronbuchwald <[email protected]>
Date:   Fri Dec 1 13:31:49 2023 -0500

    Add more descriptive formatted error (#2403)

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

commit 9b85141
Author: Dhruba Basu <[email protected]>
Date:   Thu Nov 30 12:42:36 2023 -0800

    `vms/platformvm`: Move `GetRewardUTXOs`, `GetSubnets`, and `GetChains` to `State` interface (#2402)

commit de3b16c
Author: Joshua Kim <[email protected]>
Date:   Thu Nov 30 12:49:53 2023 -0500

    Add `p2p.Network` component (#2283)

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

commit 0ab2046
Author: marun <[email protected]>
Date:   Thu Nov 30 09:18:59 2023 -0800

    Rename `testnet` fixture to `tmpnet` (#2307)

commit 907b34c
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 21:32:43 2023 -0500

    Update bootstrap IPs (#2396)

commit 96d451d
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 18:15:47 2023 -0500

    Periodically PullGossip only from connected validators (#2399)

commit 0da5bcc
Author: Dan Laine <[email protected]>
Date:   Wed Nov 29 16:32:16 2023 -0500

    Remove method `CappedList` from `set.Set` (#2395)

Signed-off-by: Joshua Kim <[email protected]>
tedim52 pushed a commit to tedim52/avalanchego that referenced this pull request Oct 9, 2024
@StephenButtolph @coffeeavax
Add AVM state.Chain interface (ava-labs#2411)
166fe00 
Co-authored-by: Chloe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@abi87 abi87 abi87 approved these changes

@danlaine danlaine danlaine approved these changes

Assignees

@dhrubabasu dhrubabasu

Labels
None yet
Projects
Platform Engineering Group
Archived in project
Milestone
v1.10.18
Development

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