feat(ICP-archive): migrate icp archive to stable structures #3910
Conversation
maciejdfinity
changed the title
| const MAX_MEMORY_SIZE_BYTES_MEMORY_ID: MemoryId = MemoryId::new(0); | ||
| const BLOCK_HEIGHT_OFFSET_MEMORY_ID: MemoryId = MemoryId::new(1); | ||
| const TOTAL_BLOCK_SIZE_MEMORY_ID: MemoryId = MemoryId::new(2); | ||
| const LEDGER_CANISTER_ID_MEMORY_ID: MemoryId = MemoryId::new(3); | ||
| const BLOCK_LOG_INDEX_MEMORY_ID: MemoryId = MemoryId::new(4); | ||
| const BLOCK_LOG_DATA_MEMORY_ID: MemoryId = MemoryId::new(5); |
There was a problem hiding this comment.
In the ICRC archive, we have a single ArchiveConfig in a StableCell. What's the rationale for having these as separate MemoryIds, and additionally, for the thread_local! _CACHE RefCells? Is it e.g., easier to add new fields if they are all separate cells? What about the cache, is that faster/more efficient in terms of instructions, or otherwise simplifying things?
There was a problem hiding this comment.
The rationale behind separate cells was that I didn't want to copy the whole state to read one integer. I am not sure how expensive that would be, since we only have 4 fields (although, the canister id is slightly larger). Maybe there is some better way, where we keep some local copy of the state and return a reference... As for the cache, I am thinking the since TOTAL_BLOCK_SIZE is modified often, the cache might not be that beneficial. However, the other fields are almost never changed, so it should be helpful there. I could run some simple benchmarks.
There was a problem hiding this comment.
I see, thanks for the explanation! I suppose we would use a similar approach for the ledger state if we move that to stable structures also (to eventually be able to get rid of the pre- and post-upgrade serialization&deserialization dance? I tried to see what is done in other canisters, and I saw that in the governance canister, there is a State struct that is used to manage the different data structures stored in individual memories in stable memory. Would it make sense to have something like that for the archive also (and later for the ledger)? It's maybe not so critical for the archive, since as you point out, we don't have so many fields, but on the other hand, maybe here it would be easier to come up with a design that we could later reuse for the ledger? On the other hand, from skimming through the governance canister, it seems they do perform non-trivial initialization in the post-upgrade, so if we want to get rid of that, maybe their approach anyway wouldn't work for us?
There was a problem hiding this comment.
Thanks for the pointer! I will have a closer look!
There was a problem hiding this comment.
Another option is to have ARCHIVE_STATE_CACHE and access it similarly as we do in the ICRC ledger, i.e. Access::with_archive_state(|state| {...}) and Access::with_archive_state_mut. The mut version would also need to update the underlying stable cell that contains the whole state.
There was a problem hiding this comment.
Or not use the cache and read the state directly. I guess if the state is large, reading the whole state might be expensive, but I don't have the exact numbers.
| if max_memory_size_bytes < total_block_size() { | ||
| ic_cdk::trap(&format!( | ||
| "Cannot set max_memory_size_bytes to {}, because it is lower than total_block_size {}.", | ||
| max_memory_size_bytes, | ||
| total_block_size() | ||
| )); | ||
| } |
There was a problem hiding this comment.
Does it make sense to have some upper limit on the max memory size? It's probably less critical for the ICP ledger archive compared to the ICRC ledger archive in the sense that whoever sets this should know what they're doing. Maybe having an alert if the size grows too big is enough.
There was a problem hiding this comment.
I think we will set the limit to something more conservative (<50G ?). We could have an alert when size reaches e.g. 50% of the limit and decide what to do then.
| setup.upgrade(Some(2 * encoded_block_size), None); | ||
| setup.assert_remaining_capacity(0); | ||
|
|
||
| setup.upgrade(Some(u64::MAX), None); |
There was a problem hiding this comment.
Regarding having a ceiling on the max size - it may be better to have something like 500GB that would allow the archive to return a more reasonable error to the ledger, so that the archive returns an error before the canister limit is reached, which may be more troublesome to handle (and test)?
The archive blocks are stored in stable log. Since migration requires only ~24B instructions, the whole migration is performed in
post_upgrade. The archive has a new argument that can be used to specify a new archive size limit. The argument can be skipped.