Skip to content

feat(ffi): add proposal root retrieval #910

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

Merged
merged 17 commits into from
Jun 2, 2025
Merged
19 changes: 5 additions & 14 deletions ffi/firewood.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (db *Database) Batch(ops []KeyValue) ([]byte, error) {
C.size_t(len(ffiOps)),
unsafe.SliceData(ffiOps), // implicitly pinned
)
return extractBytesThenFree(&hash)
return bytesFromValue(&hash)
}

func (db *Database) Propose(keys, vals [][]byte) (*Proposal, error) {
Expand All @@ -159,21 +159,12 @@ func (db *Database) Propose(keys, vals [][]byte) (*Proposal, error) {
value: values.from(vals[i]),
}
}
idOrErr := C.fwd_propose_on_db(
val := C.fwd_propose_on_db(
db.handle,
C.size_t(len(ffiOps)),
unsafe.SliceData(ffiOps), // implicitly pinned
)
id, err := extractUintThenFree(&idOrErr)
if err != nil {
return nil, err
}

// The C function will never create an id of 0, unless it is an error.
return &Proposal{
handle: db.handle,
id: id,
}, nil
return newProposal(db.handle, &val)
}

// Get retrieves the value for the given key. It always returns a nil error.
Expand All @@ -186,7 +177,7 @@ func (db *Database) Get(key []byte) ([]byte, error) {
values, cleanup := newValueFactory()
defer cleanup()
val := C.fwd_get_latest(db.handle, values.from(key))
bytes, err := extractBytesThenFree(&val)
bytes, err := bytesFromValue(&val)

// If the root hash is not found, return nil.
if err != nil && strings.Contains(err.Error(), rootHashNotFound) {
Expand All @@ -203,7 +194,7 @@ func (db *Database) Root() ([]byte, error) {
return nil, errDBClosed
}
hash := C.fwd_root_hash(db.handle)
bytes, err := extractBytesThenFree(&hash)
bytes, err := bytesFromValue(&hash)

// If the root hash is not found, return a zeroed slice.
if err != nil && strings.Contains(err.Error(), rootHashNotFound) {
Expand Down
24 changes: 15 additions & 9 deletions ffi/firewood.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@ typedef struct DatabaseHandle DatabaseHandle;
/**
* A value returned by the FFI.
*
* This is used in several different ways:
* This is used in several different ways, including:
* * An C-style string.
* * An ID for a proposal.
* * A byte slice containing data.
*
* - When returning data, the length is the length of the data and the data is a pointer to the data.
* - When returning an error, the length is 0 and the data is a null-terminated C-style string.
* - When returning an ID, the length is the ID and the data is null.
* For more details on how the data may be stored, refer to the function signature
* that returned it or the `From` implementations.
*
* The data stored in this struct (if `data` is not null) must be manually freed
* by the caller using `fwd_free_value`.
*
* A `Value` with length 0 and a null data pointer indicates that the data was not found.
*/
typedef struct Value {
size_t len;
Expand Down Expand Up @@ -290,8 +294,9 @@ const struct DatabaseHandle *fwd_open_db(struct CreateOrOpenArgs args);
*
* # Returns
*
* A `Value` containing {id, null} if creating the proposal succeeded.
* A `Value` containing {0, "error message"} if creating the proposal failed.
* On success, a `Value` containing {len=id, data=hash}. In this case, the
* hash will always be 32 bytes, and the id will be non-zero.
* On failure, a `Value` containing {0, "error message"}.
*
* # Safety
*
Expand All @@ -318,8 +323,9 @@ struct Value fwd_propose_on_db(const struct DatabaseHandle *db,
*
* # Returns
*
* A `Value` containing {id, nil} if creating the proposal succeeded.
* A `Value` containing {0, "error message"} if creating the proposal failed.
* On success, a `Value` containing {len=id, data=hash}. In this case, the
* hash will always be 32 bytes, and the id will be non-zero.
* On failure, a `Value` containing {0, "error message"}.
*
* # Safety
*
Expand Down
67 changes: 48 additions & 19 deletions ffi/firewood_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,19 @@ func kvForTest(i int) KeyValue {
// 1. By calling [Database.Propose] and then [Proposal.Commit].
// 2. By calling [Database.Update] directly - no proposal storage is needed.
func TestInsert100(t *testing.T) {
type dbView interface {
Get(key []byte) ([]byte, error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronbuchwald I think this conflicts with something you're doing, so I can get rid of this or edit if you like

Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to worry about a conflict here, will be easy to fix when we get to it

Propose(keys, vals [][]byte) (*Proposal, error)
Root() ([]byte, error)
}

tests := []struct {
name string
insert func(*Database, [][]byte, [][]byte) (root []byte, _ error)
insert func(dbView, [][]byte, [][]byte) (dbView, error)
}{
{
name: "Propose",
insert: func(db *Database, keys, vals [][]byte) ([]byte, error) {
name: "Propose and Commit",
insert: func(db dbView, keys, vals [][]byte) (dbView, error) {
proposal, err := db.Propose(keys, vals)
if err != nil {
return nil, err
Expand All @@ -219,13 +225,28 @@ func TestInsert100(t *testing.T) {
if err != nil {
return nil, err
}
return db.Root()
return db, nil
},
},
{
name: "Update",
insert: func(db *Database, keys, vals [][]byte) ([]byte, error) {
return db.Update(keys, vals)
insert: func(db dbView, keys, vals [][]byte) (dbView, error) {
actualDB, ok := db.(*Database)
if !ok {
return nil, fmt.Errorf("expected *Database, got %T", db)
}
_, err := actualDB.Update(keys, vals)
return db, err
},
},
{
name: "Propose",
insert: func(db dbView, keys, vals [][]byte) (dbView, error) {
proposal, err := db.Propose(keys, vals)
if err != nil {
return nil, err
}
return proposal, nil
},
},
}
Expand All @@ -240,20 +261,23 @@ func TestInsert100(t *testing.T) {
keys[i] = keyForTest(i)
vals[i] = valForTest(i)
}
rootFromInsert, err := tt.insert(db, keys, vals)
newDB, err := tt.insert(db, keys, vals)
require.NoError(t, err, "inserting")

for i := range keys {
got, err := db.Get(keys[i])
got, err := newDB.Get(keys[i])
require.NoErrorf(t, err, "%T.Get(%q)", db, keys[i])
// Cast as strings to improve debug messages.
want := string(vals[i])
require.Equal(t, want, string(got), "Recover nth batch-inserted value")
}

hash, err := db.Root()
hash, err := newDB.Root()
require.NoError(t, err, "%T.Root()", db)

rootFromInsert, err := newDB.Root()
require.NoError(t, err, "%T.Root() after insertion", db)

// Assert the hash is exactly as expected. Test failure indicates a
// non-hash compatible change has been made since the string was set.
// If that's expected, update the string at the top of the file to
Expand Down Expand Up @@ -415,16 +439,23 @@ func TestDeleteAll(t *testing.T) {
require.Empty(t, got, "Get(%d)", i)
}

emptyRootStr := expectedRoots[emptyKey]
expectedHash, err := hex.DecodeString(emptyRootStr)
require.NoError(t, err, "Decode expected empty root hash")

// TODO: Check proposal root
// Requires #918
// hash, err := proposal.Root()
// require.NoError(t, err, "%T.Root() after commit", proposal)
// require.Equalf(t, expectedHash, hash, "%T.Root() of empty trie", db)
Comment on lines +446 to +450
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to include this TODO here? If this requires 918, would it make more sense to include the added check in 918?

Copy link
Contributor Author

@alarso16 alarso16 May 30, 2025

Choose a reason for hiding this comment

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

#918 is an issue, but the code is trivial so we could remove that and fill it in when addressing the issue.


// Commit the proposal.
err = proposal.Commit()
require.NoError(t, err, "Commit")

// Check that the database is empty.
hash, err := db.Root()
require.NoError(t, err, "%T.Root()", db)
expectedHashHex := expectedRoots[emptyKey]
expectedHash, err := hex.DecodeString(expectedHashHex)
require.NoError(t, err)
require.Equalf(t, expectedHash, hash, "%T.Root() of empty trie", db)
}

Expand All @@ -445,27 +476,25 @@ func TestDropProposal(t *testing.T) {
err = proposal.Drop()
require.NoError(t, err, "Drop")

// Attempt to commit the dropped proposal.
// Check all operations on the dropped proposal.
err = proposal.Commit()
require.ErrorIs(t, err, errDroppedProposal, "Commit(dropped proposal)")

// Attempt to get a value from the dropped proposal.
_, err = proposal.Get([]byte("non-existent"))
require.ErrorIs(t, err, errDroppedProposal, "Get(dropped proposal)")
_, err = proposal.Root()
require.ErrorIs(t, err, errDroppedProposal, "Root(dropped proposal)")

// Attempt to "emulate" the proposal to ensure it isn't internally available still.
proposal = &Proposal{
handle: db.handle,
id: 1,
}

// Check all operations on the fake proposal.
_, err = proposal.Get([]byte("non-existent"))
require.Contains(t, err.Error(), "proposal not found", "Get(fake proposal)")

// Attempt to create a new proposal from the fake proposal.
_, err = proposal.Propose([][]byte{[]byte("key")}, [][]byte{[]byte("value")})
require.Contains(t, err.Error(), "proposal not found", "Propose(fake proposal)")

// Attempt to commit the fake proposal.
err = proposal.Commit()
require.Contains(t, err.Error(), "proposal not found", "Commit(fake proposal)")
}
Expand Down
Loading
Loading