Skip to content

Commit

Permalink
Fix 2 race conditions with proposer and transaction pool
Browse files Browse the repository at this point in the history
These were causing the "Invalid user command! Error was: Nonce in account 1
different from nonce in transaction 0" errors in fake_hash:full-test.

In proposer.ml we were getting the current best tip to propose off of it, then
waiting for an IVar, then getting the transaction pool. If the best tip changed
while we were waiting, we would see transactions that were valid against the new
best tip and potentially not the one we basing our proposal on. I changed it to
get the transaction pool at the same time as the best tip, before waiting.

We were also waiting for the proposed transition to be accepted into the
transition frontier before starting to propose again. The second race condition
is when our proposed transition has been accepted and we start to propose again
before the transaction pool is updated to remove now-invalid transactions. I
fixed by changing it to update the diffs (and consequently the transaction
pool) before calling Transition_registry.notify, which wakes up the waiting
proposer. This way the transaction pool is guaranteed to be updated in time.
  • Loading branch information
enolan committed Apr 30, 2019
1 parent c3024b9 commit 2517f9c
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 42 deletions.
24 changes: 13 additions & 11 deletions src/lib/proposer/proposer.ml
Expand Up @@ -326,6 +326,9 @@ module Make (Inputs : Inputs_intf) :
, External_transition.Verified.protocol_state_proof transition
)
in
let transactions =
Transaction_pool.transactions transaction_pool
in
trace_event "waiting for ivar..." ;
let%bind () =
Interruptible.lift (Deferred.return ()) (Ivar.read ivar)
Expand All @@ -334,9 +337,7 @@ module Make (Inputs : Inputs_intf) :
generate_next_state ~proposal_data ~previous_protocol_state
~time_controller
~staged_ledger:(Breadcrumb.staged_ledger crumb)
~transactions:
(Transaction_pool.transactions transaction_pool)
~get_completed_work ~logger ~keypair
~transactions ~get_completed_work ~logger ~keypair
in
trace_event "next state generated" ;
match next_state_opt with
Expand Down Expand Up @@ -422,16 +423,17 @@ module Make (Inputs : Inputs_intf) :
in
Logger.info logger ~module_:__MODULE__
~location:__LOC__
!"Submitting transition to the transition frontier \
controller"
!"Submitting transition $state_hash to the \
transition frontier controller"
~metadata ;
let%bind () =
Strict_pipe.Writer.write transition_writer
external_transition_with_hash
in
Logger.info logger ~module_:__MODULE__
~location:__LOC__
"Waiting for transition to be inserted into frontier" ;
~location:__LOC__ ~metadata
"Waiting for transition $state_hash to be inserted \
into frontier" ;
Deferred.choose
[ Deferred.choice
(Transition_frontier.wait_for_transition frontier
Expand All @@ -450,12 +452,12 @@ module Make (Inputs : Inputs_intf) :
| `Transition_accepted ->
Logger.info logger ~module_:__MODULE__
~location:__LOC__ ~metadata
"Generated transition was accepted into \
transition frontier"
"Generated transition $state_hash was accepted \
into transition frontier"
| `Timed_out ->
let str =
"Generated transition was never accepted into \
transition frontier"
"Generated transition $state_hash was never \
accepted into transition frontier"
in
Logger.fatal logger ~module_:__MODULE__
~location:__LOC__ ~metadata "%s" str ;
Expand Down
3 changes: 3 additions & 0 deletions src/lib/staged_ledger/staged_ledger.ml
Expand Up @@ -1672,6 +1672,9 @@ end = struct
(User_command t) )
with
| Error e ->
(* FIXME This should be fatal and crash the daemon but can't be
because of a buggy test. See #2346.
*)
Logger.error logger ~module_:__MODULE__ ~location:__LOC__
~metadata:
[ ( "user_command"
Expand Down
66 changes: 35 additions & 31 deletions src/lib/transition_frontier/transition_frontier.ml
Expand Up @@ -336,39 +336,43 @@ struct
mb_write_to_pipe diff (Field.get field t) handler pipe
in
( match diff with
| Transition_frontier_diff.New_breadcrumb breadcrumb ->
Transition_registry.notify t.transition_registry
(Breadcrumb.state_hash breadcrumb) ;
New_transition.Var.set t.new_transition
(Breadcrumb.external_transition breadcrumb) ;
New_transition.stabilize ()
| Transition_frontier_diff.New_frontier _ ->
()
| Transition_frontier_diff.New_best_tip
{ old_root
; old_root_length
; new_best_tip_length
; added_to_best_tip_path
; _ } ->
( if new_best_tip_length - old_root_length > max_length then
let root_state_hash = Breadcrumb.state_hash old_root in
Root_history.enqueue t.root_history root_state_hash old_root ) ;
let new_breadcrumb = Non_empty_list.last added_to_best_tip_path in
| Transition_frontier_diff.New_best_tip {old_root; new_root; _} ->
if not (Breadcrumb.equal old_root new_root) then
Root_history.enqueue t.root_history
(Breadcrumb.state_hash old_root)
old_root
| _ ->
() ) ;
let%map () =
Fields.fold ~init:diff
~root_history:(fun _ _ -> Deferred.unit)
~snark_pool_refcount:
(use Snark_pool_refcount.handle_diff pipes.snark_pool)
~transition_registry:(fun acc _ -> acc)
~best_tip_diff:(use Best_tip_diff.handle_diff pipes.best_tip_diff)
~root_diff:(use Root_diff.handle_diff pipes.root_diff)
~persistence_diff:
(use Persistence_diff.handle_diff pipes.persistence_diff)
~new_transition:(fun acc _ -> acc)
in
let bc_opt =
match diff with
| New_breadcrumb bc ->
Some bc
| New_best_tip {added_to_best_tip_path; _} ->
Some (Non_empty_list.last added_to_best_tip_path)
| _ ->
None
in
Option.iter bc_opt ~f:(fun bc ->
(* Other components may be waiting on these, so it's important they're
updated after the views above so that those other components see
the views updated with the new breadcrumb. *)
Transition_registry.notify t.transition_registry
(Breadcrumb.state_hash new_breadcrumb) ;
(Breadcrumb.state_hash bc) ;
New_transition.Var.set t.new_transition
(Breadcrumb.external_transition new_breadcrumb) ;
New_transition.stabilize () ) ;
Fields.fold ~init:diff
~root_history:(fun _ _ -> Deferred.unit)
~snark_pool_refcount:
(use Snark_pool_refcount.handle_diff pipes.snark_pool)
~transition_registry:(fun acc _ -> acc)
~best_tip_diff:(use Best_tip_diff.handle_diff pipes.best_tip_diff)
~root_diff:(use Root_diff.handle_diff pipes.root_diff)
~persistence_diff:
(use Persistence_diff.handle_diff pipes.persistence_diff)
~new_transition:(fun acc _ -> acc)
@@ Breadcrumb.external_transition bc ;
New_transition.stabilize () )
end

module Node = struct
Expand Down

0 comments on commit 2517f9c

Please sign in to comment.