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.

Sign up for GitHub

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

Jump to bottom

Make proposals owned by API caller #161

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Make proposals owned by API caller #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions firewood/src/v2/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
use std::{collections::HashMap, fmt::Debug, sync::Weak};
use std::{
collections::HashMap,
fmt::Debug,
sync::{Arc, Weak},
};

use async_trait::async_trait;

Expand Down Expand Up @@ -111,9 +115,9 @@ pub trait Db {
/// [BatchOp::Delete] operations to apply
///
async fn propose<K: KeyType, V: ValueType>(
&mut self,
self: Arc<Self>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

data: Batch<K, V>,
) -> Result<Weak<Self::Proposal>, Error>;
) -> Result<Arc<Self::Proposal>, Error>;
}

/// A view of the database at a specific time. These are wrapped with
Expand Down Expand Up @@ -165,12 +169,14 @@ pub trait DbView {
/// obtain proofs.
#[async_trait]
pub trait Proposal<T: DbView>: DbView {
type Proposal: DbView + Proposal<T>;

/// Commit this revision
///
/// # Return value
///
/// * A weak reference to a new historical view
async fn commit(self) -> Result<Weak<T>, Error>;
async fn commit(self) -> Result<T, Error>;
Copy link
Contributor

@richardpringle richardpringle Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when two threads try to commit two conflicting proposals at the same time?
Scenario:

P1 = R1 + [Put(K1, V1)]
P2 = R1 + [Put(K2, V2)]

t0 -> P1.commit() 
t0 -> P2.commit()

Just to be clear, these are two competing but not conflicting proposals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You actually can't create a proposal from two different revisions, unless a commit happens between them, so I'm not sure how you are creating P1 and P2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edited, should have said R1 in both cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, when P2 commits, it will see the base revision is not R1 (it is now R2) and will return an invalid proposal.


/// Propose a new revision on top of an existing proposal
///
Expand All @@ -183,7 +189,7 @@ pub trait Proposal<T: DbView>: DbView {
/// A weak reference to a new proposal
///
async fn propose<K: KeyType, V: ValueType>(
&self,
self: Arc<Self>,
data: Batch<K, V>,
) -> Result<Weak<Self>, Error>;
) -> Result<Arc<Self::Proposal>, Error>;
}
83 changes: 22 additions & 61 deletions firewood/src/v2/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use std::{
borrow::Borrow,
collections::BTreeMap,
fmt::Debug,
sync::{Arc, Mutex, RwLock, Weak},
sync::{Arc, Mutex, Weak},
};

use async_trait::async_trait;
Expand Down Expand Up @@ -39,40 +38,27 @@ impl api::Db for Db {
}

async fn propose<K: KeyType, V: ValueType>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why propose on the database at all, why not limit proposal creating to be based on revisions?

Copy link
Collaborator Author

@rkuris rkuris Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, because then you get the scenario above! You cannot propose anything except against the most recent version of the database or another proposal (which is based on the most recent version of the database). If you did, the proposal would immediately be invalid, so what would be the point?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've talked about this on calls and on slack, just wanted to get it down in GitHub somewhere as well. We should be consistent. We either put all the onus on the user to ensure they're working with valid data OR we only allow them to create valid proposals.

Currently, we enforce valid proposal creation on the database, but don't enforce anything when creating proposals on top of proposals.

I think we could actually do both, allowing the more flexible creation of proposals on top of revisions or other proposals without validating the data through trait implementations behind a commit-only-validation feature flag.

With the feature flag off, I suggest the default behaviour is a Db::propose_on(parent: Proposal, batch: Batch) -> Proposal that validates that the parent proposal is valid before creating a new proposal.

&mut self,
self: Arc<Self>,
data: Batch<K, V>,
) -> Result<Weak<Proposal>, api::Error> {
) -> Result<Arc<Proposal>, api::Error> {
let mut dbview_latest_cache_guard = self.latest_cache.lock().unwrap();

if dbview_latest_cache_guard.is_none() {
// TODO: actually get the latest dbview
*dbview_latest_cache_guard = Some(Arc::new(DbView {
proposals: RwLock::new(vec![]),
}));
*dbview_latest_cache_guard = Some(Arc::new(DbView {}));
};

let mut proposal_guard = dbview_latest_cache_guard
.as_ref()
.unwrap()
.proposals
.write()
.unwrap();

let proposal = Arc::new(Proposal::new(
let proposal = Proposal::new(
ProposalBase::View(dbview_latest_cache_guard.clone().unwrap()),
data,
));

proposal_guard.push(proposal.clone());
);

Ok(Arc::downgrade(&proposal))
Ok(Arc::new(proposal))
}
}

#[derive(Debug)]
pub struct DbView {
proposals: RwLock<Vec<Arc<Proposal>>>,
}
pub struct DbView;

#[async_trait]
impl api::DbView for DbView {
Expand Down Expand Up @@ -117,15 +103,13 @@ enum KeyOp<V: ValueType> {
pub struct Proposal {
base: ProposalBase,
delta: BTreeMap<Vec<u8>, KeyOp<Vec<u8>>>,
children: RwLock<Vec<Arc<Proposal>>>,
}

impl Clone for Proposal {
fn clone(&self) -> Self {
Self {
base: self.base.clone(),
delta: self.delta.clone(),
children: RwLock::new(vec![]),
}
}
}
Expand All @@ -142,11 +126,7 @@ impl Proposal {
})
.collect();

Self {
base,
delta,
children: RwLock::new(vec![]),
}
Self { base, delta }
}
}

Expand Down Expand Up @@ -191,35 +171,19 @@ impl api::DbView for Proposal {

#[async_trait]
impl api::Proposal<DbView> for Proposal {
type Proposal = Proposal;

async fn propose<K: KeyType, V: ValueType>(
&self,
self: Arc<Self>,
data: Batch<K, V>,
) -> Result<Weak<Self>, api::Error> {
) -> Result<Arc<Self::Proposal>, api::Error> {
// find the Arc for this base proposal from the parent
let children_guard = match &self.base {
ProposalBase::Proposal(p) => p.children.read().unwrap(),
ProposalBase::View(v) => v.proposals.read().unwrap(),
};

let arc = children_guard
.iter()
.find(|&c| std::ptr::eq(c.borrow() as *const _, self as *const _));

if arc.is_none() {
return Err(api::Error::InvalidProposal);
}

let proposal = Arc::new(Proposal::new(
ProposalBase::Proposal(arc.unwrap().clone()),
data,
));

self.children.write().unwrap().push(proposal.clone());
let proposal = Proposal::new(ProposalBase::Proposal(self), data);

Ok(Arc::downgrade(&proposal))
Ok(Arc::new(proposal))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

async fn commit(self) -> Result<Weak<DbView>, api::Error> {
async fn commit(self) -> Result<DbView, api::Error> {
todo!()
}
}
Expand All @@ -235,7 +199,6 @@ impl std::ops::Add for Proposal {
let proposal = Proposal {
base: self.base,
delta,
children: RwLock::new(Vec::new()),
};

Arc::new(proposal)
Expand All @@ -253,7 +216,6 @@ impl std::ops::Add for &Proposal {
let proposal = Proposal {
base: self.base.clone(),
delta,
children: RwLock::new(Vec::new()),
};

Arc::new(proposal)
Expand All @@ -270,7 +232,7 @@ mod test {

#[tokio::test]
async fn test_basic_proposal() -> Result<(), crate::v2::api::Error> {
let mut db = Db::default();
let db = Arc::new(Db::default());

let batch = vec![
BatchOp::Put {
Expand All @@ -280,7 +242,7 @@ mod test {
BatchOp::Delete { key: b"z" },
];

let proposal = db.propose(batch).await?.upgrade().unwrap();
let proposal = db.propose(batch).await?;

assert_eq!(proposal.val(b"k").await.unwrap(), b"v");

Expand All @@ -294,7 +256,7 @@ mod test {

#[tokio::test]
async fn test_nested_proposal() -> Result<(), crate::v2::api::Error> {
let mut db = Db::default();
let db = Arc::new(Db::default());

// create proposal1 which adds key "k" with value "v" and deletes "z"
let batch = vec![
Expand All @@ -305,17 +267,16 @@ mod test {
BatchOp::Delete { key: b"z" },
];

let proposal1 = db.propose(batch).await?.upgrade().unwrap();
let proposal1 = db.propose(batch).await?;

// create proposal2 which adds key "z" with value "undo"
let proposal2 = proposal1
.clone()
.propose(vec![BatchOp::Put {
key: b"z",
value: "undo",
}])
.await?
.upgrade()
.unwrap();
.await?;
// both proposals still have (k,v)
assert_eq!(proposal1.val(b"k").await.unwrap(), b"v");
assert_eq!(proposal2.val(b"k").await.unwrap(), b"v");
Expand Down