ADR-40 Replace IAVL and decouple state commitment from storage #8297
Replies: 30 comments 116 replies
-
|
@jlandrews, referencing your comment here: #7100 (comment), Regen Network's "orm" package automates construction of secondary indexes on top of a KV store: https://github.com/regen-network/regen-ledger/tree/master/orm. With a bit of work, it could probably do most of what's in SQL. I have proposed including it in the SDK proper (#7098). I am curious, however, if there is a viable high-performance SQL solution that could work as an alternative to a KV store or maybe on top of a KV store. I wonder for instance how much speed up there is with sqlite prepared statements vs parsing queries on demand. I personally think SQL for the storage layer would be ideal compared to what we're doing now if we could make it performant. |
Beta Was this translation helpful? Give feedback.
-
|
I think we're broadly in agreement then. My current view is that rebuilding the necessary features of SQL on a KV store is totally doable but a reasonably large engineering commitment. And that it seems unlikely that we could build something on top of a KV store which is more performant (for the same kinds of access patterns) as what SQLite is doing directly on the filesystem. |
Beta Was this translation helpful? Give feedback.
-
|
If we will support RDBMS, then most probably we can switch SQL backends. Starting with SQLite is the most reasonable options. IIRC, prepared statements save one DB call / query. |
Beta Was this translation helpful? Give feedback.
-
|
So prepared statements would save parsing and query planning on every invocation. Likely there's some non-trivial benefit. There's also other things we could do like disabling sync and journaling in memory (https://www.whoishostingthis.com/compare/sqlite/optimize/). These I think are reasonable since the block history is the source of truth. It might be interesting to do some real benchmarks to see what the performance hit really is in an optimized scenario. |
Beta Was this translation helpful? Give feedback.
-
|
Some SQL databases expose a high-performance key/value API of some sort which skips the SQL query engine entirely.
|
Beta Was this translation helpful? Give feedback.
-
|
I also believe that SQLite can provide stronger consistency guarantees than all the current supported backends. |
Beta Was this translation helpful? Give feedback.
-
|
I agree with prioritizing the separation of storage from storage commitments. And that porting SMT is probably the shortest path to a new storage commitment system. I would also propose though that there be some sort of standardized interface for what a storage commitment scheme is supposed to provide, so that future alternatives could be easily swapped in, and so that performance could be easily compared. |
Beta Was this translation helpful? Give feedback.
-
|
I would note that separating storage and commitments as proposed is a relatively big API change. Likely a valuable one, but it would mean refactoring the whole storage layer of the SDK. So a stepwise, opt-in approach makes sense. As in, we get a |
Beta Was this translation helpful? Give feedback.
-
|
I think we should be able to manage state commitments under the same |
Beta Was this translation helpful? Give feedback.
-
|
Ian asked:
Response: SMT must be small to fit in a memory. It's goal is to validate and update commitments. We need to store objects in a storage anyway to allow fast queries. Storing objects in SMT will bloat it and adds redundancy. |
Beta Was this translation helpful? Give feedback.
-
|
Simple "decoupling" of state and proofs, doesn't describe saving "old state". We need a way to store previous versions of data. This is also strongly related to "pruning" topic. Questions
Possible methods of storing old stateNaive Version PrefixEach key is prefixed with version. For each version all data belonging to that version is stored.
Database snapshotDatabase-layer snapshots used to store old versions. Version suffixEach key has a version as a suffix.
PruningSaving every n-th block
|
Beta Was this translation helpful? Give feedback.
-
|
Re Version suffix This will work for queries and updates. However it will be challenging for pruning: it would require a more sophisticated DB scan: during the scan we will need to find out which records to remove. IAVL manages version by linking trees: each version is a new tree root, which can contain old subtrees |
Beta Was this translation helpful? Give feedback.
-
CommitsOnly root commits (or main branch commits) should be written to a disk. Commits on "subversions" of |
Beta Was this translation helpful? Give feedback.
-
If we decide to save every n-th version of state, we don't need to worry about this. This is also very similar to the way Badger is storing versions/timestamps (in badger nomenclature).
Could you please define "subversions of |
Beta Was this translation helpful? Give feedback.
-
|
In SDK, the state is organized using |
Beta Was this translation helpful? Give feedback.
-
Yes. That's why I'm prefer to not do any versioning which involves DB engine. So only |
Beta Was this translation helpful? Give feedback.
-
ADR-040 draftOn Friday 15 Jan we had a kick off meeting to discuss the SMT integration in Cosmos SDK and review the ADR-040 proposal. |
Beta Was this translation helpful? Give feedback.
-
Discord ChatWe created the |
Beta Was this translation helpful? Give feedback.
-
SnapshotsNOTE: snapshot is not the same a version management. Snapshot is used for fast sync feature and recover. We don't need version management on DB level. In the ADR I propose to implement snapshots using the underlying DB engine snapshot function. This will limit the supported backends of course (Badger, RocksDB). |
Beta Was this translation helpful? Give feedback.
-
|
The nomenclature is very confusing (many conflicting names!). I have some extra questions about SDK-level snapshots:
|
Beta Was this translation helpful? Give feedback.
-
It's already coined - we have that functionality, and the package name is |
Beta Was this translation helpful? Give feedback.
-
Snapshots can be implemented in a multiple ways. But, yes - it must be consistent and atomic.
Not necessary. Snapshots are versioned by block height. When we do a snapshot, we need to know from what block height it is. |
Beta Was this translation helpful? Give feedback.
-
So, I suggest to leave snapshot management to DB (so we don't need to do "export") as much as possible. From our experience it's the best approach, because DBs have years of engineering for that (if they support that feature). We can't do a full copy - it will be to expensive. This tells me that we can't use BoltDB for that (unless we will decide to use our own mechanism). |
Beta Was this translation helpful? Give feedback.
-
Version management.NOTE: (read above): version management is not necessary same thing as snapshot. Versioning is required by SDK to process and possible roll back transaction. At minimum, AFAICT, we don't need version management on the storage engine - this could be done as it is now using However, the |
Beta Was this translation helpful? Give feedback.
-
|
I think it could be very useful to have some consensus historical states. One use case I'm thinking of is adapting the group module to calculate group weights based on token balances. With an unstaked balance, there would be a lot of problems in calculating voting weight just based on present balance if tokens are constantly being moved around. So it would be really useful for a chain to have "epochs" say every 10,000 blocks and for all chains to retain the last say 3 epoch states. Then we could define voting windows for proposals spanning specific epochs so that these token balances are all locked in at the epoch time. This may also be useful for #8328. |
Beta Was this translation helpful? Give feedback.
-
|
I don't think it's a good idea to expose past state for state machine. It would require reaching a network consensus on how many past states each node has to provide. Moreover, even if this could provide more functionality, it could also increase a complexity of module security design. I think Epoch Staking only needs to store small additional information about epochs (how long is the epoch, and when the current epoch started). @sunnya97 - do you have some comments about accessing past state? |
Beta Was this translation helpful? Give feedback.
-
|
IMO "State" is by definition that which is needed to process new transactions. So it wouldn't make sense to have transaction processing which depends on "old" state, that just means that your state needs to track multiple instances of a value. Ex: if you want epoch values of some state variable every 10000 blocks, you should have those as part of "present" state and have some way to update them when those epoch blocks are hit. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah that sums up my opinion @jlandrews. To process txs in the state machine, you should only need the current view of the latest state. All historical state is then merely optional for client purposes. If you end up needing to hold onto some data that occurred N blocks ago, e.g. epochs, then you keep that as part of the current state and delete when appropriate. |
Beta Was this translation helpful? Give feedback.
-
|
IMHO, once ADR-38 is ready we should deprecate accessing past state in the SDK, and use indexers for that. |
Beta Was this translation helpful? Give feedback.
-
|
I'm wondering if we should rework a bit the store interfaces. Mainly break the Motivation:
|
Beta Was this translation helpful? Give feedback.
-
Integrating LazyLedger SMT with Cosmos SDKWe created |
Beta Was this translation helpful? Give feedback.
-
SMT Dev Call 2021-01-28 Notes
|
Beta Was this translation helpful? Give feedback.
-
|
Here is a gist for the benchmark: var N int = 100e6 // number records
var store
func setup(){
for i:=0; i<N; i++ {
key:= hash(i)
store.Insert(key, someData)
}
}
func simulateBlock(version int){
const numOps = 2e3
for i:= 0; i<int(numOps); i++ {
idx := random(0, N)
store.Update(hash(idx), otherData) // we need to randomize it each time, maybe by adding some integer
}
// now let's add some new records:
for i := 0; i<int(numOps) / 4; i++ {
N++
store.Insert(hahs(N), otherData2)
}
store.Snapshot(version)
}
func BenchmarkSnapshots() {
setup()
sizeStart = inspect_store_size(store)
rounds := 1e3
for i:=0 ; i<rounds; i++ {
simulateBlock(version)
}
sizeMax = inspect_store_size(store)
rounds := 10e3
for i:=0 ; i<rounds/2; i++ {
store.RemoveSnapshot(i)
}
sizeAfter = inspect_store_size(store)
// Goals:
// + compare sizes
// + remove snapshots (so only keep inserts and updates) to
// check the impact of snapshots in a total performance.
}Let's comment here if this benchmark is enough to validate a feasibility of using DB snapshot mechanisms. |
Beta Was this translation helpful? Give feedback.
-
|
It would be cool to ensure that database compaction is actually executed (as a separate step, or maybe during |
Beta Was this translation helpful? Give feedback.
-
|
@tzdybal - good idea. |
Beta Was this translation helpful? Give feedback.
-
SMT Dev Call 2021-02-05 Notes
Next up:
|
Beta Was this translation helpful? Give feedback.
-
|
The proposed updates for the benchmarks are:
|
Beta Was this translation helpful? Give feedback.
-
|
Bad news is, RocksDB "snapshots" are so cheap, as they are not what they seems, and should be used differently (there is even RAII class in C++ API). I had to check sources, to find out how DB handles the snapshots - it's just an in-memory list, and internally they are used for SSI/transaction isolation. I'll reduce the number of created snapshots, and prepare a solution with RocksDB BTW - I was testing with WD Black NVMe PCI-E 3.0 x4 SSD (WDS100T2X0C). |
Beta Was this translation helpful? Give feedback.
-
What is SSI? |
Beta Was this translation helpful? Give feedback.
-
|
So, the bare snapshot in RocksDB is not durable? I thought it contains a list of files or an index file descriptor. |
Beta Was this translation helpful? Give feedback.
-
|
SSI - Serializable Snapshot Isolation. I'm 99% sure they're not durable. But "Checkpoints", definitely are. The downside of checkpoints is, they will require more "glue code" to provide access to historical state. |
Beta Was this translation helpful? Give feedback.
-
|
i think so...
Le dim. 7 févr. 2021 à 13:40, Tomasz Zdybał <[email protected]> a
écrit :
… SSI - Serializable Snapshot Isolation.
I'm 99% sure they're not durable. But "Checkpoints", definitely are. The
downside of checkpoints is, they will require more "glue code" to provide
access to historical state.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8297 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASXWWFPFDO52RDKKERVQUS3S52C23ANCNFSM4V5Y6UYQ>
.
|
Beta Was this translation helpful? Give feedback.
-
|
Serializable Snapshot Isolation.
Le sam. 6 févr. 2021 à 18:56, Robert Zaremba <[email protected]> a
écrit :
… So, the bare snapshot in RocksDB is not durable? I thought it contains a
list of files or an index file descriptor.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8297 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASXWWFLIUJO36W6NGPEYSI3S5V7ENANCNFSM4V5Y6UYQ>
.
|
Beta Was this translation helpful? Give feedback.
-
|
AvaLabs is building CedrusDB which looks like a potential candidate that would support our needs. It looks like source available soon. |
Beta Was this translation helpful? Give feedback.
-
|
I don't think it will be good for us:
We don't have independent benchmarks, but from their paper they claim to be ~30% faster (on powerful machines) than RocksDB on mixed load (%50 read +50% updates). On write only workloads rocksdb is a clear winner. On read only workloads CedrusDB is a clear winner. Again - they conducted tests on machines with lot of RAM. |
Beta Was this translation helpful? Give feedback.
-
SMT Dev Call 2021-02-12 NotesDuring the call we were looking at Badger performance and discussing the DB writes:
We also reviewed the scope of SMT changes needed to implement ADR-40. The initial list is following (this is only the SMT part, without going into SDK changes):
Next Up:
|
Beta Was this translation helpful? Give feedback.
-
|
@liamsi , @tzdybal - do you think that updating the benchmark (as noted in Next Up above) would fit in your scope? |
Beta Was this translation helpful? Give feedback.
-
Could you expand on what you mean for these two points? Is this related to https://github.com/tendermint/tm-db? |
Beta Was this translation helpful? Give feedback.
-
|
We are creating a benchmark for doing KV operations which would simulate blockchain operations (concept is here: #8297 (reply in thread)). We need to add batch updates, because in the new design we will only do batch updates. SDK is using |
Beta Was this translation helpful? Give feedback.
-
|
There is a batch api (https://github.com/tendermint/tm-db/blob/master/types.go#L78). I think the sdk should stop using tm-db and optimize for a single DB. This making dbs adhere to an interface for users to switch doesn't bring any benefits. If a user wants to use a different db they may get drastically different performance. Also we are leaving a lot of performance on the table because of this. I would propose a db pkg that defines interfaces specifically needed by the sdk and optimize for that. |
Beta Was this translation helpful? Give feedback.
-
|
Totally, this is the direction we ware taking. We were testing block sync with badgerDB and the performance went down 33% - mostly because of the misuse of it's functionality. |
Beta Was this translation helpful? Give feedback.
-
|
I had a look into https://github.com/cockroachdb/pebble and it looks to be a strong contender. Planning to add it into benchmark. |
Beta Was this translation helpful? Give feedback.
-
|
https://twitter.com/peter_szilagyi/status/1306295678138806273?s=20 "Twice as slow in speed, data storage wise it's 3-4x larger." compared to goleveldb |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for info! Maybe, at go-ethereum they are using sync operation for every update. We want to use batch operations. This may radically change the results. So, it may still be useful to benchmark it - there is a strong team behind it. I'm checking with pebble how stable it is - they don't have any release and no info about being production ready. cockroachdb/pebble#1065 |
Beta Was this translation helpful? Give feedback.
-
|
Potentially naive question: how will historical IAVL state data be handled during sync/resync? Or rather, how will the transition from IAVL to SMT be handled during a sync/resync? The AppHash in historical headers may have been generated from merkleization of the data as part of an IAVL data structure, that same data represented in SMT will not produce the same hashes. So if syncing an application's state from genesis, the app will first need to apply txs into IAVL until some point at which it switches over to using SMT? |
Beta Was this translation helpful? Give feedback.
-
|
Note @zmanian that with ADR 041 (#8646), we can now do this without dumping state to JSON. |
Beta Was this translation helpful? Give feedback.
-
|
If we convert old state data into the new store database- where the state data is no longer stored on disk as IAVL nodes but as key-value pairs- then, in addition to the SMT commitment layer which we will use going forward, do we still need an IAVL commitment layer to operate on top of that database for the historical blocks where AppHashes were produced by IAVL merkleization? |
Beta Was this translation helpful? Give feedback.
-
|
@i-norden historical queries will probably not work at all right after an upgrade so probably not. I actually wonder what problems it may cause when historical queries get broken because of in place upgrades. @zmanian do you possibly have any insights into how clients are using historical queries and what that might break? |
Beta Was this translation helpful? Give feedback.
-
|
For IAVL -> SMT upgrade, I think we will need to do a dump JSON upgrade. Rewriting data data to a new DB model will take lot of time anyway. Moreover full sync won't work unless we will add a support for both versions in one binary. |
Beta Was this translation helpful? Give feedback.
-
|
So doing a dump and reload will probably be the easiest approach here. |
Beta Was this translation helpful? Give feedback.
-
|
This weekend I read and analyzed Verkle Tire proposed by Dankrad Feist from Ethereum.
My recommendation is to stick with what we have planned. We can always update the SMT internal structure to Kate once Ethereum folks will implement it and test it. |
Beta Was this translation helpful? Give feedback.
-
Scope of work and MilestonesToday we had a meeting to plan the milestones. Notes: https://hackmd.io/rwNyKLAnQ3WRUiRH4UxJ4w?view |
Beta Was this translation helpful? Give feedback.
-
|
I read the through the milestones and they look pretty much ideal. |
Beta Was this translation helpful? Give feedback.
-
Using BadgerDB for managing version:
|
Beta Was this translation helpful? Give feedback.
-
|
cc: @ashwinphatak |
Beta Was this translation helpful? Give feedback.
-
|
@robert-zaremba , I have a question, consider the following case:
If I query with tx = db.NewTransactionAt(10) and try to get key ABC, do I get its version at height 1, or a not found error? |
Beta Was this translation helpful? Give feedback.
-
|
if the key was not removed or updated in the meantime, you will get the version at hight 1. If it was removed then not found, if updated - you will get latest version. |
Beta Was this translation helpful? Give feedback.
-
|
Ok so if no changes happen between 1-10 then I will get the latest saved version of the key compared to the height I'm querying. Thank you. |
Beta Was this translation helpful? Give feedback.
-
|
yes |
Beta Was this translation helpful? Give feedback.
-
Storage Hard Fork vs IBC and Merkle Proofs.While doing ADR-40 review, we discussed the potential problems of the state hard fork with regards to IBC. TL;DR: we need to design a mechanism for IBC which will work support hard forks anyway. Hard forks may change a module data layout, account layout or anything with a storage (even if we won't do any changes in the SMT). They are inevitable. Concerns and AnswersMerkle Proof StructureThe LazyLedger SMT has implemented functionality to construct merkle proofs. However, they are not ICS-23 compliant. We put in the scope of work to update the implementation and make it ICS-23 compliant. Storage migrationADR-40 requires a full storage migration. It's a hard fork. We didn't define the eventual update mechanism. We were thinking about doing a standard hard fork upgrade path by resetting the chain:
This will reset all light client proofs and will make IBC hard to use. Few ideas how to handle it:
Module data layout migrationModules can migrate their data layout without storage implementation change involved. This will invalidate all past merkle proofs. cc: @cwgoes , @colin-axner , @AdityaSripal |
Beta Was this translation helpful? Give feedback.
-
|
BTW - how do you support hard forks? Is there any documentation about that? |
Beta Was this translation helpful? Give feedback.
-
This should be fine if that information is encoded within the ICS-23 Proof Spec itself which I think it is. cc: @ethanfrey
The upgrade documentation is here: https://github.com/cosmos/ibc-go/tree/main/docs/upgrades |
Beta Was this translation helpful? Give feedback.
-
|
The link is broken. The correct link is: https://github.com/cosmos/ibc-go/blob/main/docs/ibc/upgrades/README.md |
Beta Was this translation helpful? Give feedback.
-
|
BTW I assume that the IAVL library will still have to be included in the SDK in order to validate the state commitments in the old blocks which are using IAVL Merkle roots, when fully syncing and validating the chain from scratch. Alternatively - perhaps full nodes can ignore IAVL state commitments until the block where SMT is introduced - if the state roots aren't being used for anything consensus-critical. |
Beta Was this translation helpful? Give feedback.
-
|
@musalbas - it will require a hard fork and genesis reset, so there won't be old blocks. |
Beta Was this translation helpful? Give feedback.
-
Yes, how we combine (key, value) (pre-hashing, length prefixing, post-hashing, etc) to form the leaf hash is defined in the ics23 spec. Note that if you switch out the underlying store, you will need to change the Proof Spec. This is tied to the client structure, but in theory should be updatable by a governance vote (like you would need to authorize after any state reset) |
Beta Was this translation helpful? Give feedback.
-
|
Why we will need to change a Proof Spec? The merkle proof should have the same structure. |
Beta Was this translation helpful? Give feedback.
-
|
Are you sure? |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for link. Why for IAVL, there is |
Beta Was this translation helpful? Give feedback.
-
|
Ask the IAVL masters. I didn't write this unspecified mess. I just decoded it almost 2 years ago. It comes down to https://github.com/cosmos/iavl/blob/master/node.go#L290-L302 I am pretty sure height and size are set to 0 in leaf nodes, thus 1 bytes, while they can be much larger for others. Also, you will find out they use protobuf's |
Beta Was this translation helpful? Give feedback.
-
Removing MultistoreMultistore is one for the relics of current store design, which has many issues (problems related to race condition and sync eg: #6370). The idea is to remove the MultiStore from the SDK, and use a single instance of Let's continue the discussion about the concept here, and in the PR let's focus more on technicalities. |
Beta Was this translation helpful? Give feedback.
-
|
The main issue as I pointed above is that we assume a way to apply the prefix to the path. Instead, we want some information from the client on how we should be applying the prefix. Either as a separate key (Multistore) or as just a prepend to the original key (SMT). The proposed fix I have in mind is to change the In the case where the spec length is 1 + merkle path length (Multistore case). We will attach the prefix as a seperate key in the Merkle path. This allows the ibc-go code to support both a Multistore chain and a SMT chain. An analysis of the security implications would be deeply appreciated since it does involve changes to Merkle proof verification |
Beta Was this translation helpful? Give feedback.
-
|
TL;DR. Switching iAVL to SMT is possible without any ibc-go changes. Removing Multistore is possible but requires ibc-go code upgrades on counterparty chains before deployment of feature Talking to @colin-axner , I believe we can probably get the simple fix in for ibc-go 1.0 and then perhaps work on a better fix later if necessary |
Beta Was this translation helpful? Give feedback.
-
|
@AdityaSripal - we can keep the same spec and use empty string as for the first element of the proof path. Will it solve the first consequence you listed in above |
Beta Was this translation helpful? Give feedback.
-
|
@colin-axner and I considered doing something like this, but it would still require changes to ibc-go to apply prefix and verify correctly. Regardless, the counterparty chains need to upgrade so it is best to choose the least hacky simple fix we can do in short term which is what I proposed above. Again, we can get this into ibc 1.0. And we can get the counterparty chains to upgrade to this before deploying the multistore removal |
Beta Was this translation helpful? Give feedback.
-
|
For any change to the multistore that requires counterparty chains to upgrade, I expect that it might take 1 year or more before all the necessary changes have landed. The Hub would like to switch from IAVL to SMT before then but if the only version of SMT doesn't contain the multistore then the Hub has to wait. |
Beta Was this translation helpful? Give feedback.
-
Removing Multistore -- using Record IDsAlternative to the prefix store based design described above for removing multistore. Copying a comment from ADR-40: use prefix store instead of multistore #9355 pull request. The concept is researched by AiB (@jgimeno , @fdymylja ): Where, NOTE: AiB proposed different interface, but the concept is similar. |
Beta Was this translation helpful? Give feedback.
-
|
Taken from PR discussion: Just as a short update on the work being done, we're moving away from the GetID interface. The user does not need to implement GetID anymore but rather just mark the PrimaryKey field during the StateObject registration and then the store implementation already knows how to encode that field (not matter what type it is) into a valid byte key. For example: https://github.com/allinbits/cosmos-sdk-poc/blob/frojdi/crisis/runtime/orm/schema/schema.go#L71 This removes the need to implement any kind of interface as now the store is smart enough to recognize, given a protobuf message, which is its primary key, how to get it and how to encode it. |
Beta Was this translation helpful? Give feedback.
-
|
@fdymylja , so the concept is to register a In |
Beta Was this translation helpful? Give feedback.
-
@robert-zaremba ATM it's not possible, you need a Secondary keys are "pointers" to primary keys. |
Beta Was this translation helpful? Give feedback.
-
Basically yes, you don't need to explicitly create a schema (from Module PoV), you just call Then the store creates the schema for the object. (The module does not need the schema object) Maybe in Options you could specify the object prefix too. And force it to be [2]byte. |
Beta Was this translation helpful? Give feedback.
-
|
I'd suggest for MultiStore to be replaced by something like an "ObjectStore" in which the KVStore follows a "folder" structure where every folder is a protobuf object. The KVStore prefix for the protobuf object does not necessarily need to be the protobuf fullname of the message itself (ex: bank.v1beta1.Balance), but rather a 2 bytes (maybe?) prefix. I'm not sure though how a module should signal that it would like to use a custom (shortened) prefix for the object instead of the object fullname. Also, registering length-variable object prefixes requires peculiar care when it comes to iterating an object prefix in order not to end up in other object domains.
Iterating over bank.Balance would make us end up in auth.Account domain too. StoreKeys of Modules have just the duty to check that a module is not trying to access another module domain of objects. I feel this could even simplify a lot genesis exporting/importing. |
Beta Was this translation helpful? Give feedback.
-
|
There are some coders, which are designed for avoid such collision. For example: we can require that domain prefixes must always have the last bit of the last byte = 1 and the last bit of any other byte =0. It still gives us lot of room: In 2 bytes we can encode 2^14 domains. BTW - I think w have a bug in the SDK - we don't check that. |
Beta Was this translation helpful? Give feedback.
-
|
Bug fixed and merged 💪 |
Beta Was this translation helpful? Give feedback.
-
|
While reviewing ADR-40 and getting more requests to let modules manages their own state and be able to only commit to some values, we propose the following update to ADR-40 to provide a low level access to State Commitment only: #9451 |
Beta Was this translation helpful? Give feedback.
-
|
cc: @roysc - let's discuss here the general approach. |
Beta Was this translation helpful? Give feedback.
-
|
I'm generally confused as to why there seems to be consensus that the following is true:
I don't see how this is true. The SMT upgrade will change the IBC-subtree from being stored under an IAVL subtree, to instead being stored under an SMT subtree. This requires chain B to know how to verify proofs from an SMT subtree. (An |
Beta Was this translation helpful? Give feedback.
-
|
Ohh, I see what you mean. Your saying that IBC proof specs are read dynamically from the configuration, and not compiled. So then, here in the client state: https://github.com/cosmos/ibc-go/blob/83315a90626edec5ec876d438c532a48c4e806be/modules/light-clients/07-tendermint/types/tendermint.pb.go#L38, the upgrade proposal can update it safely. This is because every client connection maintains a full protobuf definition of the proof specs involved, not just a string specifier for "iavl" or "simpletree". That is pretty neat that merkle tree formats for light client verification are being provided dynamically. Thats honestly incredibly impressive, amazing job! Though in the IBC code itself, I'm pretty unclear on the role of |
Beta Was this translation helpful? Give feedback.
-
|
I think @crodriguezvega is right that this will cause a problem due to how its used in ValidateSelfClient . Looking at the code, it reads as handshakes will fail at the moment due to this, in ConnOpenAck and ConnOpenTry that Carlos linked. |
Beta Was this translation helpful? Give feedback.
-
|
There don't seem to be any integration tests covering a client upgrade, with new proof specs, followed by initiating a connection between clients (at least not within ibc-go). It seems @colin-axner was the original author of this code, maybe he could shed more light on this? |
Beta Was this translation helpful? Give feedback.
-
|
Hi! I'm likely tagged as the original author because I did the code migration from the cosmos-sdk to ibc-go (and migrated the tests from x/upgrade to 02-client). Nevertheless, you are correct that there isn't a test performing the upgrade and then initiating a new connection between clients. The existing upgrade client test could be extended to check this functionality (without changing the proof spec). I believe the existing tests should give a good indication of how to test upgrading to a new proof spec. Doing actual integration tests will be difficult to do with the go testing package since you'd likely need two chains running different versions of the code (I don't think it is possible). ibc-rs might have a test for this using Hermes, but if not, the best testing approach based on current resources is to test manually With the ibc-go testing package, you can verify that the client can upgrade to the new proof spec, but I don't think you can verify that the new proof spec works after such an upgrade |
Beta Was this translation helpful? Give feedback.
-
UPDATEIBC team re-assessed and SMT update won't be compatible with chains running go-ibc < v3.0 . (still clarifying if a v3.0 will be enough for the "client" chain). |
Beta Was this translation helpful? Give feedback.
-
|
In the interim, this may help with db performance |
Beta Was this translation helpful? Give feedback.
-
|
How? |
Beta Was this translation helpful? Give feedback.
-
|
@faddat has been updating it lol 🤷 |
Beta Was this translation helpful? Give feedback.
-
|
I did mention to @faddat that it's sunsetted and that we're aiming to fix onto a single DB backend. |
Beta Was this translation helpful? Give feedback.
and others
-
IAVL has effectively become an orphaned project within the Cosmos ecosystem and it's proven to be an inefficient state commitment.
The goal of this discussion is to spec the next Cosmos-SDK solution for state-commitment and storage. It's important to treat this things separately.
References:
Beta Was this translation helpful? Give feedback.
All reactions