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.

Hmm I think #918 is a separate issue.

We have two choices - what to do when there's an empty root in firewood/ethhash mode and separately how to populate the golang proposal struct. My point only applies to the latter ie. if a nil root indicates that it must actually be represented by this zeroed out byte slice, then should we just populate it when creating the proposal instance?

Ahhh ok that makes sense. I'll address that.

ffi/memory.go

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.

ffi/proposal.go

aaronbuchwald · 2025-06-02T13:56:08Z

ffi/src/lib.rs

+/// This is used in several different ways, and should be interpreted based on the
+/// function that returned it.


Is this the best way to handle this comment? Maybe it just depends on the context and it is, but not sure as I'm reading it

aaronbuchwald · 2025-06-02T14:44:09Z

ffi/memory.go

 	// Pin the returned value to prevent it from being garbage collected.
 	defer runtime.KeepAlive(v)

 	if v == nil {
-		return errNilBuffer
+		return nil, 0, errNilBuffer


is this case 1 as well?

This should never happen, since the FFI layer is always returned an instance of the C Value, not a pointer to it. However, I thought it would be best to keep this case to avoid panics, but I can add it to the comment

aaronbuchwald · 2025-06-02T14:44:48Z

ffi/memory.go

+//	case | data    | len   | meaning
+//
+// 1.    | nil     | 0     | invalid
+// 2.    | nil     | non-0 | proposal deleted everything


to make sure I understand - the proposal ID is non-zero because it's a real proposal, but the root is empty because the trie ends up being empty as well?

Yes. The proposal ID is created from a counter initialized to 1, so we are guaranteed that it's nonzero.

aaronbuchwald · 2025-06-02T14:53:12Z

ffi/proposal.go

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


Hmm I think #918 is a separate issue.

We have two choices - what to do when there's an empty root in firewood/ethhash mode and separately how to populate the golang proposal struct. My point only applies to the latter ie. if a nil root indicates that it must actually be represented by this zeroed out byte slice, then should we just populate it when creating the proposal instance?

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

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

Closed

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

aaronbuchwald approved these changes Jun 2, 2025

View reviewed changes

alarso16 added 3 commits June 2, 2025 11:16

address comments

074c5ec

fix comments

e6afbef

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

c68a731

alarso16 enabled auto-merge (squash) June 2, 2025 15:27

alarso16 merged commit 5af8b75 into main Jun 2, 2025
26 checks passed

alarso16 deleted the alarso16/ffi-proposal-root branch June 2, 2025 15:35

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

		/// This is used in several different ways, and should be interpreted based on the
		/// function that returned it.

feat(ffi): add proposal root retrieval #910

feat(ffi): add proposal root retrieval #910

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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

alarso16 May 30, 2025 •

edited

Loading