feat(ffi): add proposal root retrieval #910

alarso16 · 2025-05-27T15:40:25Z

This is needed to enable support of Trie wrapping in geth.

ffi/proposal.go

rkuris

I would have thought that you would just change the code so that propose always returns the hash along with the proposal ID, rather than having to look things up again and dealing with errors.

rkuris

I would have thought that you would just change the code so that propose always returns the hash along with the proposal ID, rather than having to look things up again and dealing with errors.

alarso16 · 2025-05-28T16:23:58Z

ffi/src/lib.rs

+    // Get the root hash of the new proposal.
+    let mut root_hash: Value = match new_proposal.root_hash_sync().map_err(|e| e.to_string())? {
+        Some(root) => Value::from(root.as_slice()),
+        None => String::new().into(),


@rkuris This shouldn't occur in the ethhash case, but it does. Honestly I'd prefer that db.root_hash_sync() also returns nil in the no hash, but definitely out of scope. However, this must return a root for no data (the failing ethhash test)

Should be addressed in #918

alarso16 · 2025-05-29T19:22:14Z

ffi/firewood_test.go

@@ -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)


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

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

rkuris

Rust code is fine, recommended some rewording of comments.

Would be good to get Aaron to look at the go code better than me.

rkuris · 2025-05-29T19:43:49Z

ffi/firewood.h

+ * A `Value` containing {id, root} if creating the proposal succeeded.
+ * The root will always be 32 bytes, and the id will be non-zero.


I had to read this a few times; the second sentence is only true if the proposal succeeds.

Suggested change

* A `Value` containing {id, root} if creating the proposal succeeded.

* The root will always be 32 bytes, and the id will be non-zero.

* 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.

rkuris · 2025-05-29T19:44:26Z

ffi/firewood.h

@@ -318,7 +319,8 @@ struct Value fwd_propose_on_db(const struct DatabaseHandle *db,
 *
 * # Returns
 *
- * A `Value` containing {id, nil} if creating the proposal succeeded.
+ * A `Value` containing {id, root} if creating the proposal succeeded.


same as above

aaronbuchwald · 2025-05-29T19:43:15Z

ffi/proposal.go

+	// If the hash is empty, return the empty root hash.
+	if p.root == nil {
+		return make([]byte, RootLength), nil
+	}


What's the rationale to do this lazily rather than ensure it's populated with the empty root hash if needed?

This may save a memory allocation, but seems that it will likely be simpler if we just guarantee proposals are always fully populated.

There is an issue #918 for this in the ethhash front, but it might be better to enforce this on "normal" firewood as well.

aaronbuchwald · 2025-05-29T20:32:34Z

ffi/memory.go

 	}

 	// Expected empty case for Rust's `()`
 	// Ignores the length.
 	if v.data == nil {
-		return nil
+		return nil, uint32(v.len), nil


What is this case?

Seems like https://github.com/ava-labs/firewood/blob/main/ffi/src/lib.rs#L579

aaronbuchwald · 2025-05-29T20:34:34Z

ffi/memory.go

+// extractBytesAndErrorThenFree converts the cgo `Value` payload into either:
+// 1. a 32 byte slice, an id and nil error
+// 2. a nil byte slice, 0 id, and a non-nil error
+// This should only be called when the `Value` is expected to only contain an error or
+// an ID and a hash, otherwise the behavior is undefined.
+func extractBytesAndErrorThenFree(v *C.struct_Value) ([]byte, uint32, error) {


This function seems to be a bit overloaded and the comment doesn't quite align with

firewood/ffi/src/lib.rs

Line 579 in 0908305

/// - When returning an ID, the length is the ID and the data is null.

Why do we need to handle each of these cases this way? Is this intended to pass as little data as possible over the FFI boundary?

aaronbuchwald · 2025-05-29T20:39:29Z

ffi/firewood_test.go

+	// 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)


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

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

alarso16 added 3 commits May 27, 2025 11:36

add proposal root

8272677

fix

d4f31af

fix

b23483e

alarso16 commented May 27, 2025

View reviewed changes

ffi/proposal.go Show resolved Hide resolved

alarso16 added 2 commits May 27, 2025 12:00

lint

f9fc20c

lint

5a793b1

alarso16 marked this pull request as ready for review May 27, 2025 17:35

rkuris reviewed May 27, 2025

View reviewed changes

alarso16 marked this pull request as draft May 28, 2025 14:47

alarso16 added 3 commits May 28, 2025 11:16

move root creation

cae4fc6

dead code

39f0e0c

fix

2a1268c

alarso16 commented May 28, 2025

View reviewed changes

alarso16 added 3 commits May 29, 2025 15:05

Unused function

a59bc25

merge

5a4402a

Merge branch 'main' into alarso16/ffi-proposal-root

60bddcd

alarso16 commented May 29, 2025

View reviewed changes

alarso16 marked this pull request as ready for review May 29, 2025 19:33

rkuris approved these changes May 29, 2025

View reviewed changes

fix comments

5daf2fc

aaronbuchwald reviewed May 29, 2025

View reviewed changes

Comments and readability

c1891ec

alarso16 mentioned this pull request May 30, 2025

If ethhash is enabled, proposals can return None for the root hash #918

Open

Merge branch 'main' into alarso16/ffi-proposal-root

531b32a

alarso16 force-pushed the alarso16/ffi-proposal-root branch from 2a69cc4 to 531b32a Compare May 30, 2025 16:47

		* A `Value` containing {id, root} if creating the proposal succeeded.
		* The root will always be 32 bytes, and the id will be non-zero.

feat(ffi): add proposal root retrieval #910

Are you sure you want to change the base?

feat(ffi): add proposal root retrieval #910

Uh oh!

Conversation

alarso16 commented May 27, 2025

Uh oh!

Uh oh!

rkuris left a comment

Choose a reason for hiding this comment

Uh oh!

rkuris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkuris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronbuchwald May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alarso16 May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aaronbuchwald May 29, 2025 •

edited

Loading

alarso16 May 30, 2025 •

edited

Loading