Ignore oversize transaction batches #62
Conversation
|
I guessing this needs a rebase now #61 is merged? |
src/hbbft.erl
Outdated
| @@ -355,6 +355,8 @@ handle_msg(Data = #hbbft_data{round=R}, J, {dec, R, I, Share}) -> | |||
| failed_decrypt=[I|Data#hbbft_data.failed_decrypt]}); | |||
| Decrypted -> | |||
| case decode_list(Decrypted, []) of | |||
| [_ | Transactions] when length(Transactions) > Data#hbbft_data.batch_size -> | |||
| ignore; | |||
There was a problem hiding this comment.
Should we ignore or truncate here? @evanmcc thoughts?
There was a problem hiding this comment.
that's a good question! I'm not actually sure what would be best. my feeling is that either is fine? dropping makes it a lot easier to test, and maybe easier to make deterministic?
There was a problem hiding this comment.
@Vagabond If we truncate, how do we know which to drop and which to keep? If I'm following the chain of calls correctly - from here it would go into {result, {transactions, ... which the miner in turn accepts as final.
There was a problem hiding this comment.
you could just truncate to the first batch size elements, the commitment is already agreed to so everyone will have the same batch, in the same order.
Dropping is probably fine though.
xandkar
force-pushed
the
xandkar/ignore-oversize-txn-batches
branch
2 times, most recently
from
91940e6 to
fd5e3be
Compare
xandkar
changed the title
xandkar
force-pushed
the
xandkar/ignore-oversize-txn-batches
branch
from
efcc8c4 to
f677cc5
Compare
xandkar
force-pushed
the
xandkar/ignore-oversize-txn-batches
branch
from
2517352 to
1fa9fda
Compare
xandkar
force-pushed
the
xandkar/ignore-oversize-txn-batches
branch
4 times, most recently
from
c7384df to
f7216bf
Compare
| @@ -346,7 +376,20 @@ handle_msg(Data = #hbbft_data{round=R}, J, {dec, R, I, Share}) -> | |||
| check_completion(Data#hbbft_data{dec_shares=NewShares, decrypted=NewDecrypted, | |||
| failed_decrypt=[I|Data#hbbft_data.failed_decrypt]}); | |||
| Decrypted -> | |||
| #hbbft_data{batch_size=B, n=N} = Data, | |||
There was a problem hiding this comment.
Just nitpicking and I know we don't do it anywhere in the module (we should have) but it would be nice to have accessor funs for hbbft_data record imo. So like batch_size(Data), num_nodes(Data) etc perhaps? Similarly, we can then just have B div N calculated in a separate fun or whatever and check whether the transactions length exceeds.
Maybe a thing to do as a simplification pass in another PR
There was a problem hiding this comment.
I like it.
Definitely better done in a separate PR though, since it'll be a much larger diff and will further obscure the point of this one (I already did some nonessential things, like reformatting the signature (though it kind of was essential for my reading of it)).
There was a problem hiding this comment.
File an issue to remind us?
xandkar
force-pushed
the
xandkar/ignore-oversize-txn-batches
branch
from
630e165 to
ee2c80f
Compare
Pre-merge TODO:
Resolves: helium/miner#566