Skip to content
This repository has been archived by the owner on Jun 12, 2023. It is now read-only.

Fix transaction filtering in hbbft handler #72

Merged
merged 8 commits into from
Apr 17, 2019
Merged

Fix transaction filtering in hbbft handler #72

merged 8 commits into from
Apr 17, 2019

Conversation

Vagabond
Copy link
Contributor

This branch ended up being a bit exploratory, but it exposed some failures in how we were handling transactions. The actual fix is line 84 of hbbft handler, but along the way we improved some other things:

  • is_valid pre-check before speculative absorb
  • Better logging
  • A test for this issue

This depends on the corresponding blockchain-core PR helium/blockchain-core#94

@Vagabond Vagabond requested review from macpie and vihu April 16, 2019 00:20
@Vagabond
Copy link
Contributor Author

The CI failure should go away once the core PR is merged

@@ -79,13 +79,13 @@ handle_command({next_round, NextRound, TxnsToRemove, _Sync}, State=#state{hbbft=
N when N > 0 ->
Ledger0 = blockchain:ledger(Chain),
Ledger1 = blockchain_ledger_v1:new_context(blockchain_ledger_v1:delete_context(Ledger0)),

NewChain = blockchain:ledger(Ledger1, Chain),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like some comments explaining the why of each of these operations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

ok == blockchain_txn:absorb(Txn, Chain)
case blockchain_txn:is_valid(Txn, Chain) of
ok ->
case blockchain_txn:absorb(Txn, Chain) of
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no need for a new chain here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ledger here has had the context refreshed by the next_round event, we're just re-adding the pending transactions to the context (if they're still valid).

Group = ct_rpc:call(Candidate, gen_server, call, [miner, consensus_group, infinity]),
false = Group == undefined,
ok = libp2p_group_relcast:handle_command(Group, SignedTxn2),
ct_rpc:call(Candidate, sys, suspend, [Group]),
Copy link
Contributor

Choose a reason for hiding this comment

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

seen any evidence that this is racy? I guess it'd be hard to fix if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it shouldn't be, it should only race if the block timer fires between these 2 calls.

@@ -129,6 +129,73 @@ single_payment_test(Config) ->
4000 = PayerBalance + Fee,
6000 = PayeeBalance,

%% put the transaction into and then suspend one of the consensus group members
Txn2 = ct_rpc:call(Payer, blockchain_txn_payment_v1, new, [PayerAddr, PayeeAddr, 1000, Fee, 2]),
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this fail with just the absorb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug only triggers when there's transactions in the buffer that did not appear in the block. This is hard to provoke without adding latency so instead I put the transaction in the buffer of one hbbft worker, suspend it, make some blocks using the remaining consensus nodes and then resume the hbbft worker and check if the transaction gets incorrectly filtered.

@vihu vihu merged commit 88cf8bf into master Apr 17, 2019
@vihu vihu deleted the rg/valid-fix branch April 17, 2019 00:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants