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

add SetSubnetOwner to Chain interface #2031

Merged
merged 33 commits into from
Sep 28, 2023
Merged

add SetSubnetOwner to Chain interface #2031

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e11c476
move subnet owner lookup to platformvm state
dhrubabasu Sep 14, 2023
2d66725
fix tests
dhrubabasu Sep 14, 2023
e83458d
moar
dhrubabasu Sep 14, 2023
8f63ba4
Merge branch 'dev' into simplify-subnet-auth-verification
dhrubabasu Sep 15, 2023
a6364db
helper
dhrubabasu Sep 15, 2023
895f0c7
Merge branch 'dev' into simplify-subnet-auth-verification
dhrubabasu Sep 15, 2023
5faf60b
alt impl without helper (#2025)
dhrubabasu Sep 15, 2023
d65e483
add `SetSubnetOwner` to `Chain` interface
dhrubabasu Sep 15, 2023
9b5ad9e
dedupe some code
dhrubabasu Sep 15, 2023
32ce3b8
wip
dhrubabasu Sep 15, 2023
192e2f1
merged
dhrubabasu Sep 15, 2023
2a94bdf
fix merge
dhrubabasu Sep 15, 2023
7d59881
merged
dhrubabasu Sep 21, 2023
f43dc90
`blocks` -> `block`
dhrubabasu Sep 21, 2023
5e8c152
fix
dhrubabasu Sep 21, 2023
07b74c6
nit
dhrubabasu Sep 21, 2023
645013f
Merge branch 'dev' into add-support-for-setting-subnet-owner
StephenButtolph Sep 21, 2023
e0890d4
pr review
dhrubabasu Sep 21, 2023
897ac65
nit
dhrubabasu Sep 21, 2023
c374609
Merge branch 'dev' into add-support-for-setting-subnet-owner
dhrubabasu Sep 21, 2023
54e6387
nits
dhrubabasu Sep 22, 2023
b3edbeb
nit
dhrubabasu Sep 22, 2023
60836b7
Merge branch 'dev' into add-support-for-setting-subnet-owner
dhrubabasu Sep 22, 2023
f1c86ca
Merge branch 'dev' into add-support-for-setting-subnet-owner
dhrubabasu Sep 22, 2023
bef73ab
nits
dhrubabasu Sep 28, 2023
28803b6
nit
dhrubabasu Sep 28, 2023
3a9bfaf
nit
dhrubabasu Sep 28, 2023
da17042
reduce diff
dhrubabasu Sep 28, 2023
2154de3
add a UT"
dhrubabasu Sep 28, 2023
4f0124a
UT on diff
dhrubabasu Sep 28, 2023
4436795
remove `ErrCantFindSubnet`
dhrubabasu Sep 28, 2023
85afb53
nits
dhrubabasu Sep 28, 2023
c6d03d2
Merge branch 'dev' into add-support-for-setting-subnet-owner
dhrubabasu Sep 28, 2023
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
2 changes: 2 additions & 0 deletions vms/platformvm/config/execution_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ var DefaultExecutionConfig = ExecutionConfig{
ChainCacheSize: 2048,
ChainDBCacheSize: 2048,
BlockIDCacheSize: 8192,
FxOwnerCacheSize: 4 * units.MiB,
ChecksumsEnabled: false,
}

Expand All @@ -29,6 +30,7 @@ type ExecutionConfig struct {
ChainCacheSize int `json:"chain-cache-size"`
ChainDBCacheSize int `json:"chain-db-cache-size"`
BlockIDCacheSize int `json:"block-id-cache-size"`
FxOwnerCacheSize int `json:"fx-owner-cache-size"`
ChecksumsEnabled bool `json:"checksums-enabled"`
}

Expand Down
2 changes: 2 additions & 0 deletions vms/platformvm/config/execution_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func TestExecutionConfigUnmarshal(t *testing.T) {
"chain-cache-size": 6,
"chain-db-cache-size": 7,
"block-id-cache-size": 8,
"fx-owner-cache-size": 9,
"checksums-enabled": true
}`)
ec, err := GetExecutionConfig(b)
Expand All @@ -58,6 +59,7 @@ func TestExecutionConfigUnmarshal(t *testing.T) {
ChainCacheSize: 6,
ChainDBCacheSize: 7,
BlockIDCacheSize: 8,
FxOwnerCacheSize: 9,
ChecksumsEnabled: true,
}
require.Equal(expected, ec)
Expand Down
18 changes: 8 additions & 10 deletions vms/platformvm/state/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ func NewDiff(
parentID: parentID,
stateVersions: stateVersions,
timestamp: parentState.GetTimestamp(),
subnetOwners: make(map[ids.ID]fx.Owner),
}, nil
}

Expand Down Expand Up @@ -293,16 +294,6 @@ func (d *diff) AddSubnet(createSubnetTx *txs.Tx) {
if d.cachedSubnets != nil {
d.cachedSubnets = append(d.cachedSubnets, createSubnetTx)
}

castTx := createSubnetTx.Unsigned.(*txs.CreateSubnetTx)
subnetID := createSubnetTx.ID()
if d.subnetOwners == nil {
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
d.subnetOwners = map[ids.ID]fx.Owner{
subnetID: castTx.Owner,
}
} else {
d.subnetOwners[subnetID] = castTx.Owner
}
}

func (d *diff) GetSubnetOwner(subnetID ids.ID) (fx.Owner, error) {
Expand All @@ -319,6 +310,10 @@ func (d *diff) GetSubnetOwner(subnetID ids.ID) (fx.Owner, error) {
return parentState.GetSubnetOwner(subnetID)
}

func (d *diff) SetSubnetOwner(subnetID ids.ID, owner fx.Owner) {
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
d.subnetOwners[subnetID] = owner
}

func (d *diff) GetSubnetTransformation(subnetID ids.ID) (*txs.Tx, error) {
tx, exists := d.transformedSubnets[subnetID]
if exists {
Expand Down Expand Up @@ -562,5 +557,8 @@ func (d *diff) Apply(baseState State) error {
baseState.DeleteUTXO(utxoID)
}
}
for subnetID, owner := range d.subnetOwners {
baseState.SetSubnetOwner(subnetID, owner)
}
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
12 changes: 12 additions & 0 deletions vms/platformvm/state/mock_chain.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions vms/platformvm/state/mock_diff.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions vms/platformvm/state/mock_state.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

97 changes: 95 additions & 2 deletions vms/platformvm/state/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ var (
rewardUTXOsPrefix = []byte("rewardUTXOs")
utxoPrefix = []byte("utxo")
subnetPrefix = []byte("subnet")
subnetOwnerPrefix = []byte("subnetOwner")
transformedSubnetPrefix = []byte("transformedSubnet")
supplyPrefix = []byte("supply")
chainPrefix = []byte("chain")
Expand Down Expand Up @@ -115,6 +116,7 @@ type Chain interface {
AddSubnet(createSubnetTx *txs.Tx)

GetSubnetOwner(subnetID ids.ID) (fx.Owner, error)
SetSubnetOwner(subnetID ids.ID, owner fx.Owner)

GetSubnetTransformation(subnetID ids.ID) (*txs.Tx, error)
AddSubnetTransformation(transformSubnetTx *txs.Tx)
Expand Down Expand Up @@ -274,6 +276,8 @@ type stateBlk struct {
* |-. subnets
* | '-. list
* | '-- txID -> nil
* |-. subnetOwners
* | '-. subnetID -> owner
* |-. chains
* | '-. subnetID
* | '-. list
Expand Down Expand Up @@ -352,6 +356,11 @@ type state struct {
subnetBaseDB database.Database
subnetDB linkeddb.LinkedDB

// Subnet ID --> Owner of the subnet
subnetOwners map[ids.ID]fx.Owner
subnetOwnerCache cache.Cacher[ids.ID, *wrappedFxOwner] // cache of subnetID -> owner if the entry is nil, it is not in the database
Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to ignore this comment: It's consistent w/ the rest of the code but I personally don't think we need comments 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.

I agree, we'll keep it for now but I'll probably make a follow-up cleaning up the comments

subnetOwnerDB database.Database

transformedSubnets map[ids.ID]*txs.Tx // map of subnetID -> transformSubnetTx
transformedSubnetCache cache.Cacher[ids.ID, *txs.Tx] // cache of subnetID -> transformSubnetTx if the entry is nil, it is not in the database
transformedSubnetDB database.Database
Expand Down Expand Up @@ -420,6 +429,18 @@ type txAndStatus struct {
status status.Status
}

type wrappedFxOwner struct {
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
owner fx.Owner
size int
}

func wrappedFxOwnerSize(_ ids.ID, w *wrappedFxOwner) int {
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
if w == nil {
return ids.IDLen + constants.PointerOverhead
}
return ids.IDLen + w.size + constants.PointerOverhead
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand this wrapper is just to be able to use fx.Owner in a sizedCache without recalculating the size every time.
The wrapper feels like a band-aid to me. I'd add a size method to fx.Owner interface to let ever owner be used in a sizedCache. Also fx.Owner implementation may be able to calculate statically their serialized size, without need to actually serialize data

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 didn't think it made sense to have a Bytes() like method for fx.Owner since the normal usage is passing it into another struct. This is a special case since we're explicitly tracking the owners separately so I'd like to keep it contained here

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a Size() method to fx.Owner, not a Bytes() one.
I don't see why this is a special case. I can imagine other clients, possibly in other VMs may be interested in caching fx.Owners. So by having Size() method defined we'd avoid them the wrapper thing.
Also I'd like to avoid the wrapper in the P-chain. state is already pretty complex, I really don't see why we should add ad-hoc machinery to handle objects serialization and caching. We should push all of those machinery to the object themselves and slim down P-chain state.

Copy link
Contributor

@joshua-kim joshua-kim Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer implementing this wrapper as it's currently done for now and if we find that we keep needing this in other places we can consider implementing it on the fx package. I like package being able to define their own abstractions and keeping the interface as minimal as possible


func txSize(_ ids.ID, tx *txs.Tx) int {
if tx == nil {
return ids.IDLen + constants.PointerOverhead
Expand Down Expand Up @@ -573,6 +594,16 @@ func newState(

subnetBaseDB := prefixdb.New(subnetPrefix, baseDB)

subnetOwnerDB := prefixdb.New(subnetOwnerPrefix, baseDB)
subnetOwnerCache, err := metercacher.New[ids.ID, *wrappedFxOwner](
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
"subnet_owner_cache",
metricsReg,
cache.NewSizedLRU[ids.ID, *wrappedFxOwner](execCfg.FxOwnerCacheSize, wrappedFxOwnerSize),
)
if err != nil {
return nil, err
}

transformedSubnetCache, err := metercacher.New(
"transformed_subnet_cache",
metricsReg,
Expand Down Expand Up @@ -669,6 +700,10 @@ func newState(
subnetBaseDB: subnetBaseDB,
subnetDB: linkeddb.NewDefault(subnetBaseDB),

subnetOwners: make(map[ids.ID]fx.Owner),
subnetOwnerDB: subnetOwnerDB,
subnetOwnerCache: subnetOwnerCache,

transformedSubnets: make(map[ids.ID]*txs.Tx),
transformedSubnetCache: transformedSubnetCache,
transformedSubnetDB: prefixdb.New(transformedSubnetPrefix, baseDB),
Expand Down Expand Up @@ -819,8 +854,37 @@ func (s *state) AddSubnet(createSubnetTx *txs.Tx) {
}

func (s *state) GetSubnetOwner(subnetID ids.ID) (fx.Owner, error) {
if owner, exists := s.subnetOwners[subnetID]; exists {
return owner, nil
}

if wrappedOwner, cached := s.subnetOwnerCache.Get(subnetID); cached {
if wrappedOwner == nil {
return nil, database.ErrNotFound
}
return wrappedOwner.owner, nil
}

ownerBytes, err := s.subnetOwnerDB.Get(subnetID[:])
if err == nil {
var owner fx.Owner
if _, err := block.GenesisCodec.Unmarshal(ownerBytes, &owner); err != nil {
return nil, err
}
s.SetSubnetOwner(subnetID, owner)
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
s.subnetOwnerCache.Put(subnetID, &wrappedFxOwner{
owner: owner,
size: len(ownerBytes),
})
return owner, nil
}
if err != database.ErrNotFound {
return nil, err
}

subnetIntf, _, err := s.GetTx(subnetID)
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
s.subnetOwnerCache.Put(subnetID, nil)
return nil, fmt.Errorf(
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
"%w %q: %w",
ErrCantFindSubnet,
Expand All @@ -834,9 +898,14 @@ func (s *state) GetSubnetOwner(subnetID ids.ID) (fx.Owner, error) {
return nil, fmt.Errorf("%q %w", subnetID, errIsNotSubnet)
}

s.SetSubnetOwner(subnetID, subnet.Owner)
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit weird (to be performing a write on the read)... But I think it's ok

return subnet.Owner, nil
}

func (s *state) SetSubnetOwner(subnetID ids.ID, owner fx.Owner) {
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
s.subnetOwners[subnetID] = owner
}

func (s *state) GetSubnetTransformation(subnetID ids.ID) (*txs.Tx, error) {
if tx, exists := s.transformedSubnets[subnetID]; exists {
return tx, nil
Expand Down Expand Up @@ -1682,6 +1751,7 @@ func (s *state) write(updateValidators bool, height uint64) error {
s.writeRewardUTXOs(),
s.writeUTXOs(),
s.writeSubnets(),
s.writeSubnetOwners(),
s.writeTransformedSubnets(),
s.writeSubnetSupplies(),
s.writeChains(),
Expand Down Expand Up @@ -2272,6 +2342,29 @@ func (s *state) writeSubnets() error {
return nil
}

func (s *state) writeSubnetOwners() error {
for subnetID, owner := range s.subnetOwners {
subnetID := subnetID
owner := owner
Comment on lines +2338 to +2339
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought loop variables in go were created once and the value was updated with each iteration, so I thought we wouldn't need these copies here. I think it's safe to remove these lines but i could be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, it is safe to remove but I'd like to keep them since it's easier to understand that this is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the linter is going to yell at us because we are passing references to these values into functions. While it is true that those functions don't hold a reference to them... it feels like this is more clearly correct (until go 1.22)

delete(s.subnetOwners, subnetID)

ownerBytes, err := block.GenesisCodec.Marshal(block.Version, &owner)
if err != nil {
return fmt.Errorf("failed to marshal subnet owner: %w", err)
}

s.subnetOwnerCache.Put(subnetID, &wrappedFxOwner{
owner: owner,
size: len(ownerBytes),
})

if err := s.subnetOwnerDB.Put(subnetID[:], ownerBytes); err != nil {
return fmt.Errorf("failed to write subnet owner: %w", err)
}
}
return nil
}

func (s *state) writeTransformedSubnets() error {
for subnetID, tx := range s.transformedSubnets {
txID := tx.ID()
Expand Down Expand Up @@ -2349,11 +2442,11 @@ func (s *state) writeMetadata() error {
}

// Returns the block, status of the block, and whether it is a [stateBlk].
// Invariant: blkBytes is safe to parse with blocks.GenesisCodec
// Invariant: blkBytes is safe to parse with block.GenesisCodec
//
// TODO: Remove after v1.11.x is activated
func parseStoredBlock(blkBytes []byte) (block.Block, choices.Status, bool, error) {
// Attempt to parse as blocks.Block
// Attempt to parse as block.Block
dhrubabasu marked this conversation as resolved.
Show resolved Hide resolved
blk, err := block.Parse(block.GenesisCodec, blkBytes)
if err == nil {
return blk, choices.Accepted, false, nil
Expand Down
1 change: 1 addition & 0 deletions vms/platformvm/txs/executor/standard_tx_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ func (e *StandardTxExecutor) CreateSubnetTx(tx *txs.CreateSubnetTx) error {
avax.Produce(e.State, txID, tx.Outs)
// Add the new subnet to the database
e.State.AddSubnet(e.Tx)
e.State.SetSubnetOwner(txID, tx.Owner)
return nil
}

Expand Down