Skip to content
This repository has been archived by the owner on Mar 5, 2024. It is now read-only.

Dance moves disputed? Get off the Dance Floor! #1229

Merged
merged 3 commits into from
Feb 8, 2022
Merged

Conversation

michaeldjeffrey
Copy link
Contributor

@michaeldjeffrey michaeldjeffrey commented Feb 2, 2022

  • Accept a single state channel close dispute txn to move a SC to disputed, the rest will be thrown away
  • Cut rewards for disputed state channels

The added state channel test sc_dispute_prevention_test, if there are dial failures, the test will fail. It will complain about not enough summaries. I do not know how to increase the reliability at this time.

ok;
CloseState = blockchain_ledger_state_channel_v2:close_state(LedgerSC),
case {CloseState, SCDisputePrevention} of
{dispute, true} -> {error, already_disputed};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

all below is shifted over to allow for this clause.

@@ -2387,7 +2577,7 @@ add_and_gossip_fake_blocks(NumFakeBlocks, ConsensusMembers, Node, Swarm, Chain,

setup_meck_txn_forwarding(Node, From) ->
ok = ct_rpc:call(Node, meck_test_util, forward_submit_txn, [From]),
ok = ct_rpc:call(Node, blockchain_txn_mgr, submit, [fake_txn, fun(_, _) -> ok end]),
ok = ct_rpc:call(Node, blockchain_txn_mgr, submit, [fake_txn, fun(_) -> ok end]),
Copy link
Member

Choose a reason for hiding this comment

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

I think this might brake some tests as we are now using blockchain_txn_mgr:submit/2 in state channel workers

Copy link
Contributor Author

@michaeldjeffrey michaeldjeffrey Feb 2, 2022

Choose a reason for hiding this comment

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

@Vagabond
Copy link
Contributor

Vagabond commented Feb 7, 2022

This seems to do the trick.


ok = blockchain_ct_utils:wait_until_height(RouterNode, 16),

%% REVIEW: How can I assert something here about the rewards?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see here what I was expecting. Any insight would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can ask for blockchain_txn_rewards_v2:rewards_metadata and you should get back all of the gory details about rewards. It's what ETL uses to populate DB info about reward payouts.

Copy link
Contributor

@jadeallenx jadeallenx left a comment

Choose a reason for hiding this comment

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

Overall looks good. Couple of suggestions/comments.

include/blockchain_vars.hrl Outdated Show resolved Hide resolved

ok = blockchain_ct_utils:wait_until_height(RouterNode, 16),

%% REVIEW: How can I assert something here about the rewards?
Copy link
Contributor

Choose a reason for hiding this comment

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

You can ask for blockchain_txn_rewards_v2:rewards_metadata and you should get back all of the gory details about rewards. It's what ETL uses to populate DB info about reward payouts.


%% REVIEW: How can I assert something here about the rewards?
%% Nothing has been disputed yet.
{ok, Rewards1} = ct_rpc:call(RouterNode, blockchain_txn_rewards_v1, calculate_rewards, [5, 16, RouterChain]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Use v2 here b/c that's what we use in production

@evanmcc
Copy link
Contributor

evanmcc commented Feb 7, 2022

so many commits here, should probably be squashed before merge

A flood of disputes coming in from all over the place can cause enough
trouble reconciling how to pay out rewards and txns that things start to
go haywire.

- consistent arity for txn callback

- xor filters don't like to be empty

- set default sc_handler if you're not running tests with a group

- helper for adding traces

- add sc_dispute test

- break out of valid check early if the ledger state channel is already disputed

- add sc_dispute_prevention chaing var --boolean()

- add sc_dispute_prevention eunit test for rewards

- make sure dispute_prevention is active before rejecting close disputes

- Zero out when dispute prevention is active and sc is dipusted,
otherwise carry on as usual

- better searchable name for the test

- more explicit naming for internal test halpers

- add sc_dispute_prevention chain var to test suite

- Some notes and questions about this test

- grab sc_dispute_prevention from passed in Vars

- don't use assertMatch, allows empty maps to match any map

- more descriptive variable

- use rewards v2

Update src/transactions/v2/blockchain_txn_rewards_v2.erl

Co-authored-by: Andrew Thompson <andrew@hijacked.us>

sc_dispute_prevention -> sc_dispute_strategy_version

- Allows for evolution of handling state channel close disputes

- Add sc_dispute_strategy_version to rewards_vars() map

- get back metadata for rewards

- allow versions higher than 1 without code updates in these paths
is_valid check has to be done within the grace period, rewards can't be
calculated until after the grace period.

having the rewards check before will cause the test to always fail on
the is_valid check with a cannot_expire result.
@evanmcc evanmcc merged commit 8890421 into master Feb 8, 2022
@evanmcc evanmcc deleted the mj/sc-close-disputes branch February 8, 2022 01:07
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.

6 participants