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

Make the writing API of read-only databases inaccessible #8098

Open
Tracked by #7728
upbqdn opened this issue Dec 12, 2023 · 5 comments
Open
Tracked by #7728

Make the writing API of read-only databases inaccessible #8098

upbqdn opened this issue Dec 12, 2023 · 5 comments
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-rust Area: Updates to Rust code A-state Area: State / database changes

Comments

@upbqdn
Copy link
Member

upbqdn commented Dec 12, 2023

Motivation

PR #8079 adds support for opening the database in a read-only mode. However, the writing functionality still remains accessible in the API of the returned database. We should make it inaccessible for read-only databases. Note that this is not a security issue since attempting to write to a read-only database fails or panics, and the writes are not remotely triggerable.

Possible Solutions

One possible solution is having a type that implements all the read methods, but only implements the two write methods: write(batch) and spawn_format_change() when it is a read-write database. This can be implemented using a generic parameter that allows writing.

One possible implementation is adding an IsWriteable generic to the database and TypedColumnFamily, and only implementing TypedColumnFamily::for_writing() when the generic is ReadWriteDatabase.

After PR #8112, this should be implemented on TypedColumnFamily and WriteTypedBatch.

For example, the second generic parameter here is required to be DBWithThreadModeInner, we could do something similar with a ReadWriteDatabase unit struct:
https://docs.rs/rocksdb/latest/rocksdb/struct.DBCommon.html#impl-DBCommon%3CT,+DBWithThreadModeInner%3E

And here are the methods that work regardless of the generic type (for us that would be read methods):
https://docs.rs/rocksdb/latest/rocksdb/struct.DBCommon.html#impl-DBCommon%3CT,+D%3E

Credit to Teor for outlining the solutions.

Documentation

Document that secondary/read-only instances only read data that is in the database when it is opened. A specific method needs to be called to make secondary instances get more data:
https://github.com/facebook/rocksdb/wiki/Read-only-and-Secondary-instances

Document that the supported way to get read-only access to the state from a separate process is RPCs, and within the same process is cloning a ReadStateService. This is because we have a non-finalized state containing blocks not in the database.

@upbqdn upbqdn added A-rust Area: Updates to Rust code A-state Area: State / database changes labels Dec 12, 2023
@mpguerra
Copy link
Contributor

would this be a potential security issue for the blockchain scanner? If so we can add it to the list of tasks to do at some point after the MVP...

@upbqdn
Copy link
Member Author

upbqdn commented Dec 13, 2023

I don't think so because writing would fail or panic. I added this note to the description:

Note that this is not a security issue since attempting to write to a read-only database fails or panics, and the writes are not remotely triggerable.

@teor2345
Copy link
Collaborator

It might also be helpful to document that the supported way to get read-only access to the state from a separate process is RPCs, and within the same process is cloning a ReadStateService. This is because we have a non-finalized state containing blocks not in the database.

I'll add this to the ticket.

@teor2345
Copy link
Collaborator

teor2345 commented Dec 15, 2023

We might want to think about how to do this with TypedColumnFamily and WriteTypedBatch after PR #8112.

One possible implementation is adding an IsWriteable generic to the database and TypedColumnFamily, and only implementing TypedColumnFamily::for_writing() when the generic is ReadWriteDatabase.

@mpguerra mpguerra added the A-blockchain-scanner Area: Blockchain scanner of shielded transactions label Mar 6, 2024
@mpguerra
Copy link
Contributor

Is this work relevant outside of the scanner work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-blockchain-scanner Area: Blockchain scanner of shielded transactions A-rust Area: Updates to Rust code A-state Area: State / database changes
Projects
Status: Product Backlog
Development

No branches or pull requests

3 participants