Does it ever write to the DB? Why not a Arc<dyn DbReader>?

And why is the RwLock needed?

@msmouse, it doesn't ever write to the db. However, I'm trying to be consistent in the state sync and storage service code. Everything will get an Arc<RwLock<DbReaderWriter>>. The reason for this is we'll have several state sync (and storage service threads) reading and writing to the db concurrently and it's simpler to give them all a shared RwLock object than to try and do some magic around reader and writer only. IIUC, the DiemDB struct today does not have a global lock (e.g. mutex), correct? If so, I want to avoid any scary situations.

it looks like the DbReaderWriter is just

pub struct DbReaderWriter {
    pub reader: Arc<dyn DbReader>,
    pub writer: Arc<dyn DbWriter>,
}

so whoever is constructing the StorageReader just could just clone the inner Arc<dyn DbReader> and hand it down.

as far as concurrency, part of the DbReader API contract is to be thread safe, so you don't need any locks/synchronization to call it. after all, we're effectively doing read-only calls to an MVCC database here, which supports unlimited read concurrency

What happens if a single DbReader call (e.g., get_transactions()) makes several independent calls to the database (e.g., get_transactions, get_transaction_info and get_events_by_version). Each of these calls is going to read different objects in the database. MVCC might guarantee that each read works, but there's no guarantees across several independent reads, correct? A writer might write between them and violate the higher level assumptions (at the DbReader API). Likewise with interleaved writes. Note: the get_transactions() example is a little simple (because you could argue it's bound by versions, which shouldn't be changed once written, but I'm not comfortable making that assumption for everything). Perhaps this has all been thought about before, though? With a single read/write lock around the entire API, it's much easier to reason about (each API call can be thought of as executing as a single "database transaction" or atomic operation). Happy to be convinced, but I'm still not there yet 😄

The DB interfaces always take a ledger_version parameter if necessary, to guarantee a consistent view of the blockchain across reads. If not a flaw, it never assumes the user asks for "the latest".

@msmouse, yeah, my feeling is that in most cases we might be alright. But, race conditions can be very sneaky and I'm still not 100% sure we won't have issues. We're also moving to a world where state sync is going to be multi-threaded and writing to the DB concurrently (e.g., writing different epochs, account states, transactions in parallel, etc.). Thus, I want to do this simple thing now (i.e., wrap all uses of state sync and storage code in a single RwLock). If we find that the single RwLock becomes a point of contention, I'll happily optimize it, but I'd like to make sure that everything is safe and simple for now. Does that make sense?

As long as we are on the same page.

Absolutely, thanks @msmouse (and @phlip9)! I've noted this down as something to dig into in the future (once we have an initial end-to-end prototype). 😄

But still, we shouldn't need to write the DB in concurrency. It's not worth it. 😜

@msmouse.. I think we'll see. I just don't want to rule this out now, especially as our current design is going to be processing data chunks in parallel 😄

What is this API for? bootstrapping the TransactionAccumulatorSummary?

It's for the case where a node grabs all account states and needs to bootstrap its view of the transaction accumulator. However, I'll remove it for now until I'm absolutely sure we need it. I think there's a way around it.

add a pub type Result<T, E = Error> = ::std::result::Result<T, E>;

Can you elaborate? Why would we want this? These errors should be internal to the storage service only.

This I think is just more convenience along with the Error so you don't have to keep typing Error. It's optional

As weirdly as it sounds, I prefer typing Error everywhere. Especially since so much of our code uses anyhow (which I'm not a big fan of). This way, I know exactly what Error type I'm getting 😄 . It's also consistent with the rest of our code (elsewhere)

I guess we probably want handle_request to return just StorageServiceResponse and convert any errors into StorageServiceErrors? Even if DiemDB returns an error, it should probably just return an "Internal Error" variant

I've done this for now but we'll need to give it more attention in the future. For example, if some errors are really bad (e.g., internal invariant violations), we may want them to bubble up to main to cause the node to die. But we can deal with that later 😄

So, something I hadn't been thinking about before now, think about how this works out from the logging perspective, and how you find the logs with all of these crates. You'll have to check how the modules work out and hopefully they're consistent. For example, I found out there were 3 different ones for storage yesterday, and only one for say state sync

Yeah, it's not great. But, I'd argue that this is a limitation with the logger and setup and not with our code structure. For example, it's important to split the code here up because of circular dependencies and not wanting to tie the server and client together (which is what state sync currently does). When I come to add the logger I'll try to dig a little

This I think is just more convenience along with the Error so you don't have to keep typing Error. It's optional