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.

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

Drop explicit calls to chain.GetTx from txs execution paths #2431

Closed
wants to merge 66 commits into from
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
dfb74b9
introduced stakers cold attributes
abi87 Dec 6, 2023
a1fa20d
some more consolidation
abi87 Dec 6, 2023
e88b5b0
some more consolidation
abi87 Dec 6, 2023
146e4d2
removed code duplication
abi87 Dec 6, 2023
54098a1
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 7, 2023
096da56
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 9, 2023
81fa250
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 10, 2023
0e89791
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 11, 2023
3cb96d4
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 12, 2023
9b370c1
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 13, 2023
8275968
fix regression
abi87 Dec 13, 2023
749b4cc
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 13, 2023
c6edf3f
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 15, 2023
51e67fa
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 16, 2023
9437684
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 18, 2023
c5ef601
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 19, 2023
88a0060
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 20, 2023
54287cc
adding UTs for getStakerColdAttributes
abi87 Dec 20, 2023
e8305e2
some more UTs
abi87 Dec 20, 2023
c311437
renamed cold attributes to reward attributes
abi87 Dec 20, 2023
a6d02fa
some more use of staker reward attributes
abi87 Dec 20, 2023
8e24737
Merge remote-tracking branch 'upstream/dev' into pchain_drop_getTx_fr…
Dec 20, 2023
1255c4f
nits
abi87 Dec 20, 2023
de5c35c
nit
abi87 Dec 20, 2023
56474e5
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 20, 2023
1a8d48d
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Dec 27, 2023
e57b035
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 2, 2024
705d287
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 3, 2024
259cb65
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 4, 2024
bb6ce69
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 5, 2024
25285ed
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 8, 2024
3450224
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 11, 2024
b0f525b
added UTs
abi87 Jan 11, 2024
564cf01
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 12, 2024
b9939ba
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 15, 2024
b407679
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 16, 2024
5d7bd08
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 17, 2024
be4cd69
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 18, 2024
297a490
appease linter
abi87 Jan 18, 2024
98ffec0
Merge branch 'dev' into pchain_drop_getTx_from_execution_paths
abi87 Jan 19, 2024
61cfc54
fixed mock regeneration
abi87 Jan 19, 2024
60d932d
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Jan 26, 2024
f9074bb
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Feb 6, 2024
75e7183
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Feb 12, 2024
7e88b9f
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Feb 13, 2024
ce48a20
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Feb 15, 2024
66c3ff7
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Feb 19, 2024
4ee6cae
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Feb 25, 2024
0d8a51c
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 4, 2024
4b1663a
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 11, 2024
7712dea
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 19, 2024
5327899
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 21, 2024
c88b814
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 26, 2024
744b147
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 27, 2024
4137e6e
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Mar 29, 2024
06e32cd
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Apr 4, 2024
fe32ee9
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Apr 8, 2024
2fbde4b
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Apr 9, 2024
2b40578
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Apr 12, 2024
dfa9d30
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Apr 23, 2024
b546086
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 May 10, 2024
749d27a
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 May 22, 2024
6ef929e
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Jun 3, 2024
6018ab8
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Jun 11, 2024
ec4426c
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Jun 25, 2024
0dbabd7
Merge branch 'master' into pchain_drop_getTx_from_execution_paths
abi87 Jul 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions vms/platformvm/block/executor/proposal_block_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ func TestApricotProposalBlockTimeVerification(t *testing.T) {
StartTime: utx.StartTime(),
NextTime: chainTime,
EndTime: chainTime,
Priority: utx.CurrentPriority(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed for rewritten RewardStaker to pass

}).Times(2)
currentStakersIt.EXPECT().Release()
onParentAccept.EXPECT().GetCurrentStakerIterator().Return(currentStakersIt, nil)
Expand Down
98 changes: 25 additions & 73 deletions vms/platformvm/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ import (
"github.com/ava-labs/avalanchego/utils/set"
"github.com/ava-labs/avalanchego/vms/components/avax"
"github.com/ava-labs/avalanchego/vms/components/keystore"
"github.com/ava-labs/avalanchego/vms/platformvm/fx"
"github.com/ava-labs/avalanchego/vms/platformvm/reward"
"github.com/ava-labs/avalanchego/vms/platformvm/signer"
"github.com/ava-labs/avalanchego/vms/platformvm/stakeable"
"github.com/ava-labs/avalanchego/vms/platformvm/state"
"github.com/ava-labs/avalanchego/vms/platformvm/status"
Expand Down Expand Up @@ -86,16 +84,7 @@ var (
type Service struct {
vm *VM
addrManager avax.AddressManager
stakerAttributesCache *cache.LRU[ids.ID, *stakerAttributes]
}

// All attributes are optional and may not be filled for each stakerTx.
type stakerAttributes struct {
shares uint32
rewardsOwner fx.Owner
validationRewardsOwner fx.Owner
delegationRewardsOwner fx.Owner
proofOfPossession *signer.ProofOfPossession
Comment on lines -76 to -81
Copy link
Contributor Author

@abi87 abi87 Dec 6, 2023

Choose a reason for hiding this comment

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

this PR promotes this struct to state package, renaming it StakerRewardAttributes

stakerAttributesCache *cache.LRU[ids.ID, *state.StakerRewardAttributes]
}

// GetHeight returns the height of the last accepted block
Expand Down Expand Up @@ -726,44 +715,19 @@ type GetCurrentValidatorsReply struct {
Validators []interface{} `json:"validators"`
}

func (s *Service) loadStakerTxAttributes(txID ids.ID) (*stakerAttributes, error) {
// Lookup tx from the cache first.
func (s *Service) loadStakerTxAttributes(txID ids.ID) (*state.StakerRewardAttributes, error) {
// Lookup attributes from the cache first.
attr, found := s.stakerAttributesCache.Get(txID)
if found {
return attr, nil
}

// Tx not available in cache; pull it from disk and populate the cache.
tx, _, err := s.vm.state.GetTx(txID)
// attributes not available in cache; pull them from disk and populate the cache.
attr, err := s.vm.state.GetStakerRewardAttributes(txID)
if err != nil {
return nil, err
}

switch stakerTx := tx.Unsigned.(type) {
case txs.ValidatorTx:
var pop *signer.ProofOfPossession
if staker, ok := stakerTx.(*txs.AddPermissionlessValidatorTx); ok {
if s, ok := staker.Signer.(*signer.ProofOfPossession); ok {
pop = s
}
}

attr = &stakerAttributes{
shares: stakerTx.Shares(),
validationRewardsOwner: stakerTx.ValidationRewardsOwner(),
delegationRewardsOwner: stakerTx.DelegationRewardsOwner(),
proofOfPossession: pop,
}

case txs.DelegatorTx:
attr = &stakerAttributes{
rewardsOwner: stakerTx.RewardsOwner(),
}

default:
return nil, fmt.Errorf("unexpected staker tx type %T", tx.Unsigned)
}
Comment on lines -714 to -737
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this logic is being moved to state_helper.go and made available to process RewardValidatorTxs


s.stakerAttributesCache.Put(txID, attr)
return attr, nil
}
Expand Down Expand Up @@ -856,7 +820,7 @@ func (s *Service) GetCurrentValidators(_ *http.Request, args *GetCurrentValidato
return err
}

shares := attr.shares
shares := attr.Shares
delegationFee := json.Float32(100 * float32(shares) / float32(reward.PercentDenominator))

uptime, err := s.getAPIUptime(currentStaker)
Expand All @@ -869,14 +833,14 @@ func (s *Service) GetCurrentValidators(_ *http.Request, args *GetCurrentValidato
validationRewardOwner *platformapi.Owner
delegationRewardOwner *platformapi.Owner
)
validationOwner, ok := attr.validationRewardsOwner.(*secp256k1fx.OutputOwners)
validationOwner, ok := attr.ValidationRewardsOwner.(*secp256k1fx.OutputOwners)
if ok {
validationRewardOwner, err = s.getAPIOwner(validationOwner)
if err != nil {
return err
}
}
delegationOwner, ok := attr.delegationRewardsOwner.(*secp256k1fx.OutputOwners)
delegationOwner, ok := attr.DelegationRewardsOwner.(*secp256k1fx.OutputOwners)
if ok {
delegationRewardOwner, err = s.getAPIOwner(delegationOwner)
if err != nil {
Expand All @@ -894,7 +858,7 @@ func (s *Service) GetCurrentValidators(_ *http.Request, args *GetCurrentValidato
ValidationRewardOwner: validationRewardOwner,
DelegationRewardOwner: delegationRewardOwner,
DelegationFee: delegationFee,
Signer: attr.proofOfPossession,
Signer: attr.ProofOfPossession,
}
reply.Validators = append(reply.Validators, vdr)

Expand All @@ -907,7 +871,7 @@ func (s *Service) GetCurrentValidators(_ *http.Request, args *GetCurrentValidato
if err != nil {
return err
}
owner, ok := attr.rewardsOwner.(*secp256k1fx.OutputOwners)
owner, ok := attr.RewardsOwner.(*secp256k1fx.OutputOwners)
if ok {
rewardOwner, err = s.getAPIOwner(owner)
if err != nil {
Expand Down Expand Up @@ -1064,15 +1028,15 @@ func (s *Service) GetPendingValidators(_ *http.Request, args *GetPendingValidato
return err
}

shares := attr.shares
shares := attr.Shares
delegationFee := json.Float32(100 * float32(shares) / float32(reward.PercentDenominator))

connected := s.vm.uptimeManager.IsConnected(nodeID, args.SubnetID)
vdr := platformapi.PermissionlessValidator{
Staker: apiStaker,
DelegationFee: delegationFee,
Connected: connected,
Signer: attr.proofOfPossession,
Signer: attr.ProofOfPossession,
}
reply.Validators = append(reply.Validators, vdr)

Expand Down Expand Up @@ -1957,17 +1921,12 @@ func (s *Service) GetBlockchainStatus(r *http.Request, args *GetBlockchainStatus
}

func (s *Service) nodeValidates(blockchainID ids.ID) bool {
chainTx, _, err := s.vm.state.GetTx(blockchainID)
subnetID, err := s.vm.state.GetChainSubnet(blockchainID)
if err != nil {
return false
}

chain, ok := chainTx.Unsigned.(*txs.CreateChainTx)
if !ok {
return false
}

_, isValidator := s.vm.Validators.GetValidator(chain.SubnetID, s.vm.ctx.NodeID)
_, isValidator := s.vm.Validators.GetValidator(subnetID, s.vm.ctx.NodeID)
return isValidator
}

Expand All @@ -1984,15 +1943,14 @@ func (s *Service) chainExists(ctx context.Context, blockID ids.ID, chainID ids.I
}
}

tx, _, err := state.GetTx(chainID)
if err == database.ErrNotFound {
switch _, err := state.GetChainSubnet(chainID); {
case err == nil:
return true, nil
case errors.Is(err, database.ErrNotFound):
return false, nil
}
if err != nil {
default:
return false, err
}
_, ok = tx.Unsigned.(*txs.CreateChainTx)
return ok, nil
}

// ValidatedByArgs is the arguments for calling ValidatedBy
Expand Down Expand Up @@ -2340,12 +2298,12 @@ func (s *Service) GetStake(_ *http.Request, args *GetStakeArgs, response *GetSta
continue
}

tx, _, err := s.vm.state.GetTx(staker.TxID)
stakerRewardAttributes, err := s.vm.state.GetStakerRewardAttributes(staker.TxID)
if err != nil {
return err
}

stakedOuts = append(stakedOuts, getStakeHelper(tx, addrs, totalAmountStaked)...)
stakedOuts = append(stakedOuts, getStakeHelper(stakerRewardAttributes.Stake, addrs, totalAmountStaked)...)
}

pendingStakerIterator, err := s.vm.state.GetPendingStakerIterator()
Expand All @@ -2361,12 +2319,12 @@ func (s *Service) GetStake(_ *http.Request, args *GetStakeArgs, response *GetSta
continue
}

tx, _, err := s.vm.state.GetTx(staker.TxID)
stakerRewardAttributes, err := s.vm.state.GetStakerRewardAttributes(staker.TxID)
if err != nil {
return err
}

stakedOuts = append(stakedOuts, getStakeHelper(tx, addrs, totalAmountStaked)...)
stakedOuts = append(stakedOuts, getStakeHelper(stakerRewardAttributes.Stake, addrs, totalAmountStaked)...)
}

response.Stakeds = newJSONBalanceMap(totalAmountStaked)
Expand Down Expand Up @@ -2789,17 +2747,11 @@ func (s *Service) getAPIOwner(owner *secp256k1fx.OutputOwners) (*platformapi.Own
return apiOwner, nil
}

// Takes in a staker and a set of addresses
// Takes in a slice of reward attributes and a set of addresses
// Returns:
// 1) The total amount staked by addresses in [addrs]
// 2) The staked outputs
func getStakeHelper(tx *txs.Tx, addrs set.Set[ids.ShortID], totalAmountStaked map[ids.ID]uint64) []avax.TransferableOutput {
staker, ok := tx.Unsigned.(txs.PermissionlessStaker)
if !ok {
return nil
}

stake := staker.Stake()
func getStakeHelper(stake []*avax.TransferableOutput, addrs set.Set[ids.ShortID], totalAmountStaked map[ids.ID]uint64) []avax.TransferableOutput {
abi87 marked this conversation as resolved.
Show resolved Hide resolved
stakedOuts := make([]avax.TransferableOutput, 0, len(stake))
// Go through all of the staked outputs
for _, output := range stake {
Expand Down
2 changes: 1 addition & 1 deletion vms/platformvm/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func defaultService(t *testing.T) (*Service, *mutableSharedMemory) {
return &Service{
vm: vm,
addrManager: avax.NewAddressManager(vm.ctx),
stakerAttributesCache: &cache.LRU[ids.ID, *stakerAttributes]{
stakerAttributesCache: &cache.LRU[ids.ID, *state.StakerRewardAttributes]{
Size: stakerAttributesCacheSize,
},
}, mutableSharedMemory
Expand Down
8 changes: 8 additions & 0 deletions vms/platformvm/state/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,10 @@ func (d *diff) GetCurrentStakerIterator() (StakerIterator, error) {
return d.currentStakerDiffs.GetStakerIterator(parentIterator), nil
}

func (d *diff) GetStakerRewardAttributes(stakerID ids.ID) (*StakerRewardAttributes, error) {
return getStakerRewardAttributes(d, stakerID)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be calling the parent state GetStakerRewardAttributes? It feels strange that we can return staker reward attributes on a tx that only exists on Diff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In current implementation this is a "derivative" method. The recursion to parent state happens by GetTx, which is called internally to getStakerRewardAttributes`.
So current implementation does not really need these methods; it can just use GetTx and do the processing of the tx at client level.
This PR helps moving P-chain to the merkleDB where we don't want txs to be part of the merkle state.

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment was around the implementation of this method on Diff. It does not seem correct to return staker reward attributes if the tx only exists on Diff and not on the parentState. I think this function should be:

Suggested change
return getStakerRewardAttributes(d, stakerID)
parentState, ok := d.stateVersions.GetState(d.parentID)
if !ok {
return nil, fmt.Errorf("%w: %s", ErrMissingParentState, d.parentID)
}
return parentState.GetStakerRewardAttributes(stakerID)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the tx only exists on Diff and not on the parentState

I don't think this is correct. getStakerRewardAttributes(d, stakerID) calls d.GetTx(stakerID) internally. d.GetTx will look for the tx both in the diff and in the parent state.
I added a couple of UTs named getStakerRewardAttributes works across layers and getChainSubnet works across layers to show this. Let me know if they are convincing.

}

func (d *diff) GetPendingValidator(subnetID ids.ID, nodeID ids.NodeID) (*Staker, error) {
// If the validator was modified in this diff, return the modified
// validator.
Expand Down Expand Up @@ -330,6 +334,10 @@ func (d *diff) AddChain(createChainTx *txs.Tx) {
}
}

func (d *diff) GetChainSubnet(chainID ids.ID) (ids.ID, error) {
Copy link
Contributor

@dhrubabasu dhrubabasu Dec 27, 2023

Choose a reason for hiding this comment

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

Why are we allowing calling this method on a Diff? Should we reduce this to only being on state.State?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the API it's used over the diff too. See func (s *Service) chainExists which is called by func (s *Service) GetBlockchainStatus API

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it- I think we should call the parentState here too

return getChainSubnet(d, chainID)
}

func (d *diff) GetTx(txID ids.ID) (*txs.Tx, status.Status, error) {
if tx, exists := d.addedTxs[txID]; exists {
return tx.tx, tx.status, nil
Expand Down
Loading
Loading