sea-orm integration + rust-lightning persitence layer#95
sea-orm integration + rust-lightning persitence layer#95dcorral wants to merge 1 commit intoRGB-Tools:masterfrom
Conversation
dcorral
force-pushed
the
rust-lightning-tx-persistence
branch
3 times, most recently
from
60ab584 to
8aceb21
Compare
|
@dcorral could you please rebase this PR? |
dcorral
force-pushed
the
rust-lightning-tx-persistence
branch
from
8aceb21 to
f71fb32
Compare
|
@zoedberg done! |
dcorral
force-pushed
the
rust-lightning-tx-persistence
branch
10 times, most recently
from
f5f35f3 to
84b0895
Compare
dcorral
force-pushed
the
rust-lightning-tx-persistence
branch
from
84b0895 to
b67a1ff
Compare
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
Thanks! In addition to the requested changes please
- commit the
src/database/entities/prelude.rsfile that gets generated by sea-orm-cli - add the migration cargo lock file to the
.gitignorefile and remove the file - do not make changes to
src/database/entities/mod.rssince its autogenerated by sea-orm-cli - I know you took inspiration from rgb-lib when creating aliases like ChannelPeerActMod, but I saw this is not the best/recommended approach so please drop the aliases (see rgb-multisig-hub to see what is the best way to use the DB utility objects)
- as requested in the rust-lightning PR, please keep only essential comments
P.S this is not a final review, will do another one after these requests have been addressed
| // Use single connection to avoid deadlocks | ||
| opt.max_connections(1) | ||
| .min_connections(0) | ||
| .connect_timeout(Duration::from_secs(8)) | ||
| .idle_timeout(Duration::from_secs(8)) | ||
| .max_lifetime(Duration::from_secs(8)); |
There was a problem hiding this comment.
| // Use single connection to avoid deadlocks | |
| opt.max_connections(1) | |
| .min_connections(0) | |
| .connect_timeout(Duration::from_secs(8)) | |
| .idle_timeout(Duration::from_secs(8)) | |
| .max_lifetime(Duration::from_secs(8)); | |
| opt.max_connections(8) | |
| .min_connections(1) | |
| .connect_timeout(Duration::from_secs(8)) | |
| .idle_timeout(Duration::from_mins(5)) | |
| .max_lifetime(Duration::from_hours(1)); |
| .expect("Failed to create database tokio runtime") | ||
| }); | ||
|
|
||
| pub(crate) fn block_on<F>(future: F) -> F::Output |
There was a problem hiding this comment.
I think we should use the DB in an async way (see comment in the rust-lightning PR) and drop this entire file
|
|
||
| // Initialize the shared database connection | ||
| let db_path = get_db_path(&args.storage_dir_path); | ||
| let connection_string = format!("sqlite:{}?mode=rwc", db_path.display()); |
There was a problem hiding this comment.
I don't think it's safe to use db_path.display(), please see how we do this in rgb-lib (adjust_canonicalization)
| .integer() | ||
| .not_null() | ||
| .primary_key() | ||
| .default(1), |
There was a problem hiding this comment.
I don't think we should set defaults for the indexes
| .table(Mnemonic::Table) | ||
| .if_not_exists() | ||
| .col( | ||
| ColumnDef::new(Mnemonic::Id) |
There was a problem hiding this comment.
let's call the indexes Idx, since that's how we did on all our other projects
| manager | ||
| .create_table( | ||
| Table::create() | ||
| .table(Mnemonic::Table) |
There was a problem hiding this comment.
Can't we save the mnemonic in the Config table?
| .not_null() | ||
| .primary_key(), | ||
| ) | ||
| .col(ColumnDef::new(Config::Value).string().not_null()) |
There was a problem hiding this comment.
Instead of saving values as strings in multiple rows I think it's better to have a single row DB table with one column per value using the appropriate type for each column (see how we did this in rgb-multisig-hub)
| /// Save config to database (source of truth) and sync to file for rust-lightning compatibility. | ||
| fn save_config_and_sync_file( |
There was a problem hiding this comment.
We don't need backwards compatibility so please drop all the files (mnemonic, bitcoin network etc)
| let result = block_on( | ||
| ChannelPeerEntity::find() | ||
| .filter(ChannelPeerColumn::Pubkey.eq(pubkey)) | ||
| .one(self.get_connection()), | ||
| )?; | ||
|
|
||
| if let Some(peer) = result { | ||
| block_on(peer.delete(self.get_connection()))?; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think here you can just do delete_many().filter(...) instead of reading and then deleting
2 participants
Summary
Replaces file-based persistence with SQLite database using sea-orm ORM.
Changes
New database tables:
mnemonic- encrypted mnemonic storagekv_store- LDK persistence (channel manager, payments, swaps, network graph, scorer)config- configuration key/value pairs (indexer_url, proxy_url, etc.)revoked_token- revoked auth token IDschannel_peer- channel peer pubkey/address mappingMigrated to database:
Architecture:
DatabaseConnectionviaArcSeaOrmKvStoreimplements rust-lightning'sKVStoreSynctraitrust-lightning submodule (
961313c→b061470):KVStoreSynctrait across 8 filesrgb_utils/mod.rs: replaced file I/O (read_rgb_transfer_info,write_rgb_transfer_info,parse_rgb_payment_info,get_rgb_channel_info_path) with KVStore namespace-based read/write operations usingRGB_PRIMARY_NS+ secondary namespaces (channel_info,payment_info_inbound,payment_info_outbound,transfer_info, etc.)ChannelContext,ChannelManager,OutboundPayments,KeysManager: addedrgb_kv_store: Arc<dyn KVStoreSync + Send + Sync>field, threaded through constructors and deserializationcolor_commitment,color_htlc,color_closing: now take&dyn KVStoreSyncinstead of reading files directlyis_channel_rgb,get_rgb_channel_info,rename_rgb_files,filter_first_hops,is_payment_rgb: switched from path-based lookups to KVStore readsIMPORTANT!
This PR depends on RGB-Tools/rust-lightning#17 and .submodule must be updated to point to the right RGB-Tools fork before merging