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

blockchain: Add UtxoBackend interface. #2652

Merged
merged 12 commits into from
May 25, 2021
Merged

Conversation

rstaudt2
Copy link
Member

This reworks the UTXO related logic to use a UtxoBackend interface. It is broken up into several commits to ease the review process as things moved around.

High-level design (BlockChain / UtxoViewpoint -> UtxoCacher -> UtxoBackend)

  • BlockChain and UtxoViewpoint interact with the UtxoCacher interface and do not directly interact with UtxoBackend or the underlying database
  • UtxoCache interacts with the UtxoBackend interface and does not directly interact with the underlying database
  • UtxoBackend encapsulates the logic that is specific to the backend implementation being used
  • utxoio.go now only contains database-agnostic serialization functions, and anything specific to the underlying database was moved to utxobackend.go

Motivation

  • The flow is simplified, as above, which makes the logic easier to follow and understand what belongs where
  • A next step will be to optimize the LevelDbUtxoBackend implementation in terms of memory usage and processing time (with the initial approach being to use leveldb directly instead of the existing database.DB interface). The changes required for that can now be localized to LevelDbUtxoBackend without having to change much else

Additional Notes

  • One exception to the above design is that the upgrade logic in upgrade.go still deals with the UTXO database directly
    • I considered adding something along the lines of Cursor and Tx interfaces and associated methods to UtxoBackend in this PR to allow UtxoBackend to be used here rather than the UTXO database directly
    • I opted not to do this in this PR because the interface design would need to fit with what the database.DB interface is currently doing, which we will be moving away from shortly. So it will make more sense to add this functionality to the interface after moving away from the database.DB interface
  • Another UtxoBackend interface method to query the existence of a specific UTXO without retrieving it would be useful
    • This was brought up by @davecgh previously and would allow for some optimizations to avoid actually loading the UTXO and could make use of APBFs
    • I didn't add this here since there are already quite a few changes, but it makes sense to add it as a follow up separately

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I've reviewed about half of this so far. The direction looks like what we discussed and I really appreciate the separation of the commits to make the review easier, especially for larger changes such as these.

I figured I'd go ahead and post the few things I've spotted so far while I review the remaining commits.

}
key := outpointKey(outpoint)
err = utxoBucket.Put(*key, serialized)
// NOTE: The key is intentionally not recycled here since the database
Copy link
Member

Choose a reason for hiding this comment

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

I realize that as of this commit and PR, it's still actually using a database.DB behind the scenes, so this has to remain and is definitely true for now, but I figured I'd point it out as something to keep in mind when replacing the backend as I would expect this to no longer necessarily be the case for a new backend that is designed specifically for the utxo set that doesn't go through the database.DB interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, agreed. That is a good call-out.

blockchain/utxobackend.go Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/utxobackend_test.go Outdated Show resolved Hide resolved
blockchain/utxocache.go Outdated Show resolved Hide resolved
blockchain/chain.go Outdated Show resolved Hide resolved
blockchain/chainio.go Outdated Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/utxobackend.go Outdated Show resolved Hide resolved
blockchain/upgrade.go Outdated Show resolved Hide resolved
blockchain/upgrade.go Outdated Show resolved Hide resolved
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I've finished reviewing everything. It looks great overall. Once the review items are addressed, I'll put it through the paces.

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

I've reviewed all of the updates. I'll be testing this over the next few days, but I don't expect any issues based on the review.

This removes the dependency that the UtxoCacher interface and UtxoCache
implementation currently have on the database.Tx interface.  This is
necessary since the UtxoCache will move away from the current database
interface in favor of an interface that is better suited for the UTXO
database in a subsequent commit.
This adds a UtxoBackend interface and LevelDbUtxoBackend type, which
implements that interface.  This UtxoBackend interface will allow
different backend implementations to be used by the UtxoCache rather
than interacting directly with the database layer as it does currently.

In subsequent commits, interface methods and the corresponding
implementation will be added to UtxoBackend one at a time in order to
ease the review process as things move around.
This exports the UtxoSetState type since it will be used in the
UtxoBackend interface in a subsequent commit.
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Great!

blockchain/utxocache.go Outdated Show resolved Hide resolved
This adds the FetchEntry method to the UtxoBackend interface and
LevelDbUtxoBackend implementation.

A summary of the changes is as follows:
- Add FetchEntry method to UtxoBackend interface
- Add FetchEntry method to LevelDbUtxoBackend
- Move dbFetchUtxoEntry to LevelDbUtxoBackend
- Add backend tests for FetchEntry
- Update UtxoCache to fetch entries from the backend rather than from
  the database directly
This adds the PutUtxos method to the UtxoBackend interface and
LevelDbUtxoBackend implementation.

A summary of the changes is as follows:
- Add PutUtxos method to UtxoBackend interface, which atomically updates
  the UTXO set and state
- Add PutUtxos method to LevelDbUtxoBackend
- Move dbPutUtxoEntry to LevelDbUtxoBackend
- Add backend tests for PutUtxos
- Update UtxoCache to use PutUtxos rather than updating the database
  directly
This adds the FetchState method to the UtxoBackend interface and
LevelDbUtxoBackend implementation.

A summary of the changes is as follows:
- Add FetchState method to UtxoBackend interface
- Add FetchState method to LevelDbUtxoBackend
- Add backend tests for FetchState
- Update UtxoCache to use FetchState rather than querying the database
  directly
- Remove dbPutUtxoSetState and dbFetchUtxoSetState since they are no
  longer used
- Add FetchBackendState to the UtxoCache so that the state can be
  fetched through the cache
This adds the FetchStats method to the UtxoBackend interface and
LevelDbUtxoBackend implementation.

A summary of the changes is as follows:
- Add FetchStats method to UtxoBackend interface
- Add FetchStats method to LevelDbUtxoBackend
- Move dbFetchUtxoStats to LevelDbUtxoBackend
- Add FetchStats to the UtxoCache so that the stats can be fetched
  through the cache
This adds the FetchInfo, InitInfo, and PutInfo methods to the
UtxoBackend interface and LevelDbUtxoBackend implementation.

A summary of the changes is as follows:
- Add FetchInfo, InitInfo, and PutInfo methods to UtxoBackend interface
- Add FetchInfo, InitInfo, and PutInfo methods to LevelDbUtxoBackend
- Move dbPutUtxoDatabaseInfo, dbFetchUtxoDatabaseInfo, and associated
  variables to LevelDbUtxoBackend
- Move initUtxoDbInfo, createUtxoDbInfo, and associated variables to
  LevelDbUtxoBackend
- Rename and export utxoDatabaseInfo to UtxoBackendInfo
- Add backend tests for putting and fetching the UTXO backend info
- Add utxoBackend to BlockChain Config since BlockChain handles
  initializing the UTXO backend
- Remove utxoDbInfo from BlockChain and instead access the UTXO backend
  info through the UTXO backend
This moves LoadUtxoDB and the associated helper functions and variables
from utxodb.go to utxobackend.go since LoadUtxoDB is specific to the
LevelDbUtxoBackend implementation of the UtxoBackend interface.
This adds the Upgrade method to the UtxoBackend interface and
LevelDbUtxoBackend implementation.

A summary of the changes is as follows:
- Add Upgrade method to UtxoBackend interface
- Add Upgrade method to LevelDbUtxoBackend
- Remove initUtxoState and instead handle upgrading through the
  UtxoCache Initialize method
- Pass the UTXO backend into upgradeUtxoDb rather than the entire
  BlockChain instance
This removes the UTXO db from the BlockChain and UtxoCache types.
BlockChain now only accesses the UTXO db through the UtxoCache and the
UtxoCache now only accesses the UTXO db through the UtxoBackend.
This exports the ViewFilteredSet type since it is used in the UtxoCacher
interface.
@davecgh davecgh merged commit 840401e into decred:master May 25, 2021
@rstaudt2 rstaudt2 deleted the utxo-backend branch July 2, 2021 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants