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.

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

Apply scope and coroutines to compute tests #76

Merged
merged 13 commits into from
Sep 23, 2024

Conversation

stephenctw
Copy link
Contributor

Each Lua player is wrapped in a coroutine with isolated scope.

@stephenctw stephenctw self-assigned this Sep 15, 2024
Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

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

Looking good! Left a few comments.


local player_start_index = 2
local blockchain_node = Blockchain:new()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking now, this module could be under tests too, instead of client-lua; the whole client-lua/blockchain/ should be under tests.

Comment on lines 79 to 81
local deploy_cmd = [[sh -c "cd ../../contracts && ./deploy_anvil.sh"]]
local reader = io.popen(deploy_cmd)
local pid = assert(reader):read()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we move the blockchain modules to tests, I think we should move put code on this code on a module there too.

else
all_idle_count = 0
if status == "dead" then
table.remove(player_coroutines, i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what happens if we remove an entry in the middle of a loop. Note that this function moves back all the elements after i.

You could assign false, and add a check at the beginning (skipping the body if it's "falsy").

This would break the way you're checking whether the program is finished. An alternative way to check this is using a logic similar to the one in the example.

Copy link
Contributor Author

@stephenctw stephenctw Sep 16, 2024

Choose a reason for hiding this comment

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

Good catch. I think if I iterate the table in backwards, I get to keep the table.remove and the same logic for checking whether the program is finished. Do you think it's a bad idea to iterate the coroutines in backward manner? It'll always start with sybil player or idle player in this setup. Or we could of course prepare the players in backward so the order of starting the players remains the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could just skip dead coroutines, and keep track of whether there is at least one live coroutine, in order to break from the while true loop, like:

while true do
  local has_live_couroutine = false
  for i, c in ipairs(player_coroutines) do
    -- ...
    if status == "dead"
      goto continue
    end
    has_live_couroutine = true
    -- ...
    ::continue::
  end
  if not has_live_couroutine then
    break
  end
  -- ...
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a way of determining whether the honest player won. At the end of everything, we should verify whether the dispute actually finished (by querying the blockchain), and whether the accepted final hash was indeed the expected one. The first one should be easy. The second you'd probably need to store the honest player in some variable, so that you can later on get its commitment, and figure out the final state. I don't know.

Copy link
Contributor Author

@stephenctw stephenctw Sep 16, 2024

Choose a reason for hiding this comment

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

This is interesting. We should also consider the case when we are running the Rust dispute node. It might just be easy to write to some file at a designated path, where all the temporary snapshots are stored for the current tournament.

@@ -1,7 +1,7 @@
local consts = require "computation.constants"
local MerkleBuilder = require "cryptography.merkle_builder"
local Hash = require "cryptography.hash"
local CommitmentBuilder = require "computation.commitment"
local new_scoped_require = require "utils.scoped_require"
Copy link
Collaborator

@GCdePaula GCdePaula Sep 15, 2024

Choose a reason for hiding this comment

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

It's ok to sandbox the dishonest players from each other, but it might make sense to allow the dishonest players to "coordinate" and see each other's states too.

Copy link
Collaborator

@GCdePaula GCdePaula Sep 15, 2024

Choose a reason for hiding this comment

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

One way to implement these fake players is by calculating the honest commitment once, and then doing splices on it.

For example, suppose you know the honest commitment. The way change only the final state is the following (not tested!!):

local function change_last(root, new_last)
  local left, right = root:children()
  if not right then
    return new_last
  else
    return Hash:join(left, change_last(right, new_last))
  end
end

The above function receives a commitment and returns a new commitment, with only the last state different. Here's a way to make it arbitrary (not tested!!):

local function change_leaf(root, height, new_leaf, index)
  if height == 0 then
    return new_leaf
  else
    local left, right = root:children()
    assert(left); assert(right)
    if (index >> (height-1)) & 1 == 0
      return Hash:join(change_leaf(left, height-1, new_leaf, index), right)
    else
      return Hash:join(left, change_leaf(right, height-1, new_leaf, index))
    end
  end
end

Copy link
Contributor Author

@stephenctw stephenctw Sep 16, 2024

Choose a reason for hiding this comment

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

It's ok to sandbox the dishonest players from each other, but it might make sense to allow the dishonest players to "coordinate" and see each other's states.

This is exactly the case here, I'm only creating separate scopes to isolate the honest commitment calculation as part of the fake commitment construction to avoid the sybil player to know and act as if it's being honest.

The steps are like this:

  • The honest commitment is calculated in isolation
  • The sybil player receives the honest commitment and construct the fake ones by swapping a leaf out of the honest one.
  • The tricky part is that we need to rebuild the trees, and reconstruct Hash objects with its current scope because all the ones from honest commitment are different Hash class from other scope, and may cause unexpected issues if we just mix them together. This is mostly what rebuild_nested_trees and update_scope_of_hashes are doing.

Copy link
Contributor Author

@stephenctw stephenctw Sep 18, 2024

Choose a reason for hiding this comment

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

Hey @GCdePaula , I really like your algorithm and I think it is very efficient in the way that it only rebuilds the branch of the tree that contains the modified leaf and update the root hash accordingly. However things are a bit more tricky since the honest commitment is calculated in a separate scope now, so it becomes inevitable to rebuild the whole thing using the Hash from fake commitment scope, to rebuild the children relationship. To apply your algorithm, the honest commitment must not be done in a separate scope, this may be doable since I added the extra check on the player's turn of the match. But I think you like it being isolated because it's how things should behave in the real world in production. We can schedule a call to discuss this if you think is necessary.

-- restore the internal states of `Hash` from previous snapshot
-- this is to avoid the dishonest player to react on the honest behave
-- Hash.set_internal(new_internalized_hashes)
local c = coroutine.create(function(...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function doesn't need to be variadic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I just accept auto-completion too quickly without checking.

@@ -2,46 +2,35 @@
require "setup_path"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this require "setup_path" may not be needed anymore. Can you test?

Also the #!/usr/bin/lua maybe?

@@ -15,12 +38,23 @@ local function new_scoped_require(env)

local function scoped_require(name)
if loaded[name] == nil then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change this line to just if not loaded[name] then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great changes!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, otherwise the cartesi module cannot be loaded!

prt/tests/compute/blockchain/utils.lua Show resolved Hide resolved
prt/tests/compute/prt_compute.lua Outdated Show resolved Hide resolved
else
all_idle_count = 0
if status == "dead" then
table.remove(player_coroutines, i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could just skip dead coroutines, and keep track of whether there is at least one live coroutine, in order to break from the while true loop, like:

while true do
  local has_live_couroutine = false
  for i, c in ipairs(player_coroutines) do
    -- ...
    if status == "dead"
      goto continue
    end
    has_live_couroutine = true
    -- ...
    ::continue::
  end
  if not has_live_couroutine then
    break
  end
  -- ...
end

prt/tests/compute/utils/scoped_require.lua Outdated Show resolved Hide resolved
@stephenctw
Copy link
Contributor Author

Also added Rust compute node as a coroutine. There are two versions:

  • Rust compute node reacts once to the tournament and then exits
  • Rust compute node reacts periodically to the tournament

The first version doesn't perform well because it has to re-compute all the commitments everytime the process starts. It can improve a lot once we are able to cache and restore the commitments.

Copy link
Collaborator

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

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

✨✨✨

@stephenctw stephenctw merged commit e6de9bd into refactor/compute-tests Sep 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants