Thv/external guessers #478

Sword-Smith · 2025-02-28T00:16:57Z

Add infrastructure (RPC endpoints) to allow guessing by other programs than neptune-core. Still missing: CLI endpoints for this feature, and callbacks when a new block proposal is received. But this is a good start.

jan-ferdinand

I can't comment on the architectural design since I'm too unfamiliar with the architecture of Neptune core. The construction can benefit from a few additional iterations.

jan-ferdinand · 2025-02-28T08:28:32Z

src/main_loop.rs

+        // This block spans global state write lock for updating.
+        let mut global_state_mut = self.global_state_lock.lock_guard_mut().await;
+
+        if !global_state_mut.incoming_block_is_more_canonical(&new_block) {


The pattern if !some_bool { a() } else { b() } inhibits readability. I suggest to either use if some_bool { b() } else { a() } or to name the boolean such that negation happens somewhere else.

src/main_loop.rs

src/rpc_server.rs

jan-ferdinand · 2025-02-28T09:08:49Z

src/rpc_server.rs

+        let Some(mut proposal) = self
+            .state
+            .lock_guard()
+            .await


The lock guard is acquired multiple times. Can this be factored out into a variable?

That's intentional. To return as quickly as possible and release all locks as quickly as possible.

jan-ferdinand · 2025-02-28T09:09:10Z

src/rpc_server.rs

+        let Some(proposal) = self
+            .state
+            .lock_guard()
+            .await


The lock guard is acquired multiple times. Can this be factored out into a variable?

jan-ferdinand · 2025-02-28T09:10:12Z

src/rpc_server.rs

+        let (guesser_key_after_image, latest_block_header) = {
+            let state = self.state.lock_guard().await;
+            let guesser_key_after_image = state
+                .wallet_state
+                .wallet_secret
+                .guesser_spending_key(proposal.header().prev_block_digest);
+            let latest_block_header = *state.chain.light_state().header();
+
+            (guesser_key_after_image.after_image(), latest_block_header)
+        };


Why so roundabout?

Suggested change

let (guesser_key_after_image, latest_block_header) = {

let state = self.state.lock_guard().await;

let guesser_key_after_image = state

.wallet_state

.wallet_secret

.guesser_spending_key(proposal.header().prev_block_digest);

let latest_block_header = *state.chain.light_state().header();

(guesser_key_after_image.after_image(), latest_block_header)

};

let state = self.state.lock_guard().await;

let guesser_key_after_image = state

.wallet_state

.wallet_secret

.guesser_spending_key(proposal.header().prev_block_digest)

.after_image();

let latest_block_header = *state.chain.light_state().header();

Why so roundabout?

just some background:

The first way drops the lock guard when the end of (inner) scope is reached, releasing the lock.

Your suggestion keeps the lock around until the end of outer scope. It can be explicitly dropped with drop(state) but the suggestion does not do that, so it actually changes behavior (assuming there is subsequent code in the outer scope).

We always want to drop the locks as quickly as possible after usage, and never hold them across lengthy operations, or a second attempt to acquire the lock. There are basically 3 ways to acquire/use/drop:

in a single statement: global_state_lock.lock_guard().do_something()

in a new scope: {let state = global_state_lock.lock_guard(); do_something()}

with a drop: let state = global_state_lock.lock_guard(); do_something(); drop(state);

I tend to prefer (1) and (2) and use (3) somewhat sparingly. I find that (3) is more likely to lead to drop() being forgotten or later removed, while (2) makes the drop automatic and impossible to forget.

The advantage of an explicit drop(…) is that is communicates what is happening instead of relying on implicit behavior. I think this helps readability and maintainability; scoping might simplify initial construction.

I personally am less likely to remove a drop(…) than I am to rewrite an oddly scoped block that looks like an artifact of a refactoring tool (in this case, inlining a function).

Those are valid points. However removing a drop() during maintenance is only half of it. The other half is remembering to put it there in the first place, and to keep it in the right place as code is refactored. If one uses the convention that every time we acquire a lock guard for use by 2 or more statements we start a new scope, then there is less chance of forgetting to add the drop, and unwittingly hold it across a lengthy operation.

Either way, it is pattern/convention within the codebase that requires some familiarity to get right. Comments are helpful, especially for new eyes to the codebase. And we should always pay extra attention to any lock changes in PRs.

jan-ferdinand · 2025-02-28T09:18:12Z

src/rpc_server.rs

+        let Some(proposal) = self
+            .state
+            .lock_guard()
+            .await
+            .block_proposal
+            .map(|x| x.to_owned())
+        else {
+            return Ok(None);
+        };


Suggested change

let Some(proposal) = self

.state

.lock_guard()

.await

.block_proposal

.map(|x| x.to_owned())

else {

return Ok(None);

};

let state = self.state.lock_guard().await;

let Some(proposal) = state.block_proposal.map(|x| x.to_owned()) else {

return Ok(None);

};

again, be mindful that the suggested change is not dropping the acquired lock. I haven't checked subsequent code, so maybe it's ok, but it is a logic change.

aszepieniec

The word "challenge" to describe the information needed to completely specify a computational task and test candidate solutions is a poor choice of words and I strongly suggest finding an alternative.

In computational complexity and in cryptography the word used to describe this information is problem. More specifically it is a search problem (as opposed to a decision problem; because the output is more than one bit). Opinions differ about what the thing being searched for ought to be called, but the feature of hash functions that makes them suitable for use in proof-of-work is correlation intractability. That said, I'm not sure a scientifically accurate term is the best choice of words for type, variable, and function names.

In the context of proof-of-work, and particularly in less scientific and more layperson-friendly literature, I've seen "proof-of-work puzzle" used often. I suggest to go with this phrase.

The reason why "challenge" is a poor choice is because in cryptography and decentralized protocols it denotes one step of an interactive challenge and response protocol. A verifier sets the challenge and scrutinizes the response in order to verify the authenticity of prior or subsequent messages, or the authorization of the responder. These features do not describe proof-of-work.

src/main_loop.rs

src/models/channel.rs

src/rpc_server.rs

aszepieniec · 2025-03-02T22:03:47Z

src/rpc_server.rs

+    /// Provide a PoW-solution to the current block proposal.
+    ///
+    /// Returns true iff the provided solution satisfies the proof-of-work
+    /// threshold of the next block. False otherwise. False can be returned if
+    /// the solution comes in too late in which case either a new block
+    /// proposal is stored by the client, or no block proposal is known at all.


or*

With that correction I prefer Ferdinand's phrasing.

About the part where false is returned when there is a new block proposal -- is that really the desired behavior? IIuc, it happens when

the RPC client has a valid solution to the proof-of-work puzzle, but

neptune-core has already switched to a new block proposal, probably because it is more favorable.

Isn't the 100% certainty of getting the less favorable guesser reward preferable to X% certainty of getting the more favorable guesser reward? It is possible that X (the proportion of the guessing power owned by the user) and the favorability difference is such that it is rational to ignore own proof-of-work solutions, but it seems like a very poor heuristic to default to.

aszepieniec · 2025-03-02T22:14:55Z

src/rpc_server.rs

+        };
+
+        // A proposal was found. Check if solution works.
+        let (guesser_key_after_image, latest_block_header) = {


Binding a value of type SpendingKey to a variable named *_after_image seems like a category error.

Oh, I see. You're calling .after_image() which makes the value bound to this variable of type Digest. So that checks out.

I'm sure there is a less confusing way of structuring the code, or maybe even just variable naming. At any rate, the same criticism survives for the variable name two lines lower.

Agree with the variable-renaming. The code, though, is structured this way to minimize the time that read locks are held ,and to using scoping rules to release locks.

aszepieniec · 2025-03-02T22:20:56Z

src/rpc_server.rs

+
+        // No time to waste! Inform main_loop!
+        let solution = Box::new(proposal);
+        let _ = self


There is not much you can do besides

killing the RPC server task (probably not a good idea since the channel might be temporarily out of capacity)

logging an error message (probably a good idea).

src/rpc_server.rs

aszepieniec · 2025-03-02T22:38:21Z

src/rpc_server.rs

+        }
+
+        #[tokio::test]
+        async fn exported_guess_challenge_is_consistent_with_block_hash() {


Is "exported" the right word? When I hear it I think of "export to PDF".

What does this test achieve that guess_challenge_info_is_consistent_with_block_hash does not?

guess_challenge_info_is_consistent_with_block_hash (two tests above) tests the logic of the data structure holding the data of the puzzle. This test tests the entire pipeline.

Add an RPC endpoint that allows for the exporting of the guesser challenge to an external program.

Add endpoint `pow_solution` to provide a solution to the proof-of-work challenge. This can be used by external programs or networks that do the guessing.

cf: #478

Sword-Smith · 2025-03-03T18:46:25Z

Integrated most reviewer feedback. Still releasing locks as soon as possible, though. Let's discuss the issue @aszepieniec raised: What to do with the raise condition where the client registers a new block proposal (not a new block) and the external guesser (simultaneously) finds the block for the old block proposal that no longer is stored in the client's state.

The solution I've come up with is to store exported block proposals to a file somewhere.

Sword-Smith requested a review from aszepieniec February 28, 2025 00:16

jan-ferdinand reviewed Feb 28, 2025

View reviewed changes

aszepieniec requested changes Mar 2, 2025

View reviewed changes

Sword-Smith added 3 commits March 3, 2025 18:33

feat(rpc_server): Allow exporting of guesser challenge

8ab2e7e

Add an RPC endpoint that allows for the exporting of the guesser challenge to an external program.

feat(rpc_server): Add endpoint to provide PoW solution

1d145d3

Add endpoint `pow_solution` to provide a solution to the proof-of-work challenge. This can be used by external programs or networks that do the guessing.

style: Integrate reviewer feedback

16b92aa

cf: #478

Sword-Smith force-pushed the thv/external-guessers branch from 6001e8c to 16b92aa Compare March 3, 2025 18:45

Sword-Smith requested a review from aszepieniec March 3, 2025 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thv/external guessers #478

Thv/external guessers #478

Sword-Smith commented Feb 28, 2025 •

edited

Loading

jan-ferdinand left a comment

jan-ferdinand Feb 28, 2025 •

edited

Loading

jan-ferdinand Feb 28, 2025

Sword-Smith Mar 3, 2025

jan-ferdinand Feb 28, 2025

jan-ferdinand Feb 28, 2025

dan-da Mar 1, 2025

jan-ferdinand Mar 1, 2025 •

edited

Loading

dan-da Mar 3, 2025

jan-ferdinand Feb 28, 2025

dan-da Mar 1, 2025

aszepieniec left a comment

aszepieniec Mar 2, 2025

aszepieniec Mar 2, 2025

aszepieniec Mar 2, 2025

Sword-Smith Mar 3, 2025 •

edited

Loading

aszepieniec Mar 2, 2025

aszepieniec Mar 2, 2025

Sword-Smith Mar 3, 2025

Sword-Smith commented Mar 3, 2025 •

edited

Loading

Thv/external guessers #478

Are you sure you want to change the base?

Thv/external guessers #478

Conversation

Sword-Smith commented Feb 28, 2025 • edited Loading

jan-ferdinand left a comment

Choose a reason for hiding this comment

jan-ferdinand Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-ferdinand Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aszepieniec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sword-Smith Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sword-Smith commented Mar 3, 2025 • edited Loading

Sword-Smith commented Feb 28, 2025 •

edited

Loading

jan-ferdinand Feb 28, 2025 •

edited

Loading

jan-ferdinand Mar 1, 2025 •

edited

Loading

Sword-Smith Mar 3, 2025 •

edited

Loading

Sword-Smith commented Mar 3, 2025 •

edited

Loading