Service transactions consume account nonces!

[afck]

There are currently at least three sources of transactions for a validator's account:

• Malice reports create a transaction and put it in the queue.
• AuthorityRound::on_prepare_block pushes calls to the randomness and validator set contract directly onto a new block before transactions from the queue are added.
• The user is supposed to be able to make transactions (stake, withdraw, or do anything unrelated to POSDAO).

Whenever one of these is created while another one is still in flight (hasn't been added to a block), they will share the same account nonce and one of them will get rejected.

So firstly, we need to make sure malice reports are handled the same way as the transactions in on_prepare_block. Instead of being added to the queue as transactions, the node could collect its own pending malice reports and then add them as a transaction as part of on_prepare_block (where the account nonce counter is incremented with each new transaction).

So firstly, we need to keep count of the number of malice reports that are in flight, to give them distinct nonces. Whenever we author a block, we should push all our own pending malice reports onto it before the commitHash/emitInitiateChange/… calls in on_prepare_block.

Secondly, we should either:

• Keep validator nodes' mining accounts separate from their users' accounts. Block rewards and staking tokens should be received and held by the latter, while the former would deal with the above service transactions and never have any funds.
• Or we could not use transactions for the nodes' own contract calls at all: Instead, the nodes would put the relevant information into the block header and the contracts would be called implicitly by the system. We would need to find a way around the problem that those can't emit events, for InitiateChange. We'd also have to handle proper validation of the header data ourselves.

Edit: So the current plan is the following.

1. When making a reportMalicious/reportBenign transaction, don't use the current account nonce, but keep count of which nonces we already used (in any transaction, not just a report). Both calls use Client::transact, to which we could just add another optional parameter that would be used as the nonce instead of the "current" one.
2. Either make sure that these transactions stay in our queue until they are confirmed, and find a way to retrieve our own pending transactions (where the sender is the mining address) from the queue; or keep a separate queue with our own transactions.
3. In AuthorityRound::on_prepare_block, first add all our own pending transactions, in the correct order. (Nonces must always be consecutive!) Then increase the nonce accordingly before adding the randomness and validator set calls. (Should the "global" nonce count be incremented in on_prepare_block or only after that block has been sealed and added? Probably it has to be the latter, because on_prepare_block is called way too often.)

Edit: Or we just keep a list of malice reports, i.e. (validator, block_number) tuples in AuthorityRound. We'd periodically create new transactions with the current nonce for them, knowing that some of them might fail. So:

• Add a list of (Address, BlockNumber) tuples to AuthorityRound (or the validator set?), possibly with some timeout?
• Whenever we create a malice report transaction, add the validator and block number to the list.
• Periodically re-send transactions reporting every entry (validator, block_number) in the list for which maliceReportedForBlock(validator, block_number) called on the latest block does not contain our own mining key.
• Whenever a block is finalized, remove every entry from the list for which on that block maliceReportedForBlock(validator, block_number) does contain our own mining key.

(Let's not do this for benign reports yet. Possibly add a TODO to the code to either remove them entirely or later treat them the same way as malice reports.)

[varasev]

So firstly, we need to make sure malice reports are handled the same way as the transactions in on_prepare_block.

Unfortunately, this won't help us because there can be a case when validator had reported but after that was removed from the set of validators (at the end of staking epoch). In that case, on_prepare_block (and thus reportMalicious) won't be called on the turn of the removed validator.

Keep validator nodes' mining accounts separate from their users' accounts. Block rewards and staking tokens should be received and held by the latter, while the former would deal with the above service transactions and never have any funds.

It seems each validator needs to have three addresses:

1. Mining address.

This address could be used to call commitHash, revealSecret, and emitInitiateChange with service transaction (with zero gas price) as it happens now.

As far as I understand, calling those functions in the same block with correct nonce is implemented correctly in the current code in aura-pos branch.

2. Reporting address.

This address could be used only for calling reportMalicious with service transaction (with zero gas price).

With TxPermission.allowedTxTypes function we could prevent using this address for other purposes. So, allowedTxTypes would only allow to call ValidatorSetAuRa.reportMalicious for the reporting address.

3. Staking address.

This address could be used by validator for calling stake, withdraw, moveStake functions of ValidatorSetAuRa contract and for any other transactions with a non-zero gas price (not related to POSDAO contracts).

The only caveat here is about reporting address: what if this address calls reportMalicious or reportBenign several times per block? (e.g. for the different reported validators) Could we deal with the nonce in Parity code for that case?

[varasev]

The only caveat here is about reporting address: what if this address calls reportMalicious or reportBenign several times per block? Could we deal with the nonce in Parity code for that case?

Another question here: we expect that reportMalicious will only be called by a node, but in fact, it can be called by validator directly. This also can get us to the problem with nonce. Does anyone have any thoughts on how to restrict calling this function directly?

It seems that an ideal solution for this whole situation would be to call reportMalicious by system. Is it difficult to implement?

At the moment reportMalicious seems to be implemented incorrectly even in original Parity: it doesn't take into account that validator can call it directly and that validator can have zero balance (thus, doesn't have an ability to call it and report about malicious validator).

[afck]

Good idea! Three addresses would solve the issue, I think. 👍

Could we deal with the nonce in Parity code for that case?

Yes, I think so. The node would just need to keep count of the transactions it sent.
(And possibly we need to handle the special case where some transaction never gets committed, and retry or something.)

Does anyone have any thoughts on how to restrict calling this function directly?

If the validator somehow manually creates a transaction using the reporting address, then they (the human) misbehaved, and thus cause their node to misbehave (fail to make another report). Not sure if we need to handle this case explicitly.

call reportMalicious by system. Is it difficult to implement?

I'd say it's basically impossible, for the reason you gave above: the node may try to make the report towards the end of the staking epoch, and never create a block again. So it would be unable to report as the system, even if we put the information into the block header. (Except if it sent it to another validator and that validator would make the system call—but that would essentially mean duplicating transaction and queue functionality.)

it doesn't take into account that validator can call it directly and that validator can have zero balance

Right, we need to make the gas price zero there (and whitelist it, if it isn't already).

[varasev]

If the validator somehow manually creates a transaction using the reporting address, then they (the human) misbehaved, and thus cause their node to misbehave (fail to make another report). Not sure if we need to handle this case explicitly.

Let's assume that we have some malicious validator A and their friend - validator B.

The validator B (the friend of the malicious validator A) can call reportMalicious manually at the same block when the node of B automatically calls reportMalicious to report about A.

Assume, that in such a case the validator B uses the nonce issue and calls reportMalicious with some fake parameters (another _maliciousValidator or _blockNumber) not to let their node B automatically report about malicious validator A.

Then, the malicious validator A won't be reported by their friend B.

So, I think this might be a problem.

However, on the other hand, the malicious validator A has to have in friends at least 50% of validators to not to be reported using this scheme (because of this condition).

Also, instead of that scheme, the friends of the malicious validator A could just turn off their nodes for some short period of time right after the block when A misbehaved to not to report about A.

This problem seems to be mostly related to a 51% attack.

What do you think?

I'd say it's basically impossible, for the reason you gave above: the node may try to make the report towards the end of the staking epoch, and never create a block again.

Let's assume that the current staking epoch finishes at the block #100.

Assume that some malicious validator A misbehaves right at the latest block #100 of staking epoch (right before the block #101 at which new validator set is applied and finalizeChange is called).

Also assume that we have some honest validator C which will be removed from validator set by the algorithm at the beginning of the new staking epoch at the block #101 (i.e. C was a validator on block #100 but won't be a validator on block #101).

It seems that the regular transaction reportMalicious will anyway be called on the block #101 by the node C even when the C is not validator any more.

Could you check Parity code to confirm this is true?

In the case of the system call for the situation above the reportMalicious won't be called on the block #101 by the node C because the C is not a validator at this block anymore. Is that exactly what you meant?

Right, we need to make the gas price zero there (and whitelist it, if it isn't already).

Yes, we have already done it in #59 and the validators' addresses are whitelisted by Certify and TxPermission contracts to be able to use zero gas price to call the functions of ValidatorSetAuRa and RandomAuRa contracts.

[varasev]

Yes, I think so. The node would just need to keep count of the transactions it sent.
(And possibly we need to handle the special case where some transaction never gets committed, and retry or something.)

And again: is it really possible in Parity code for those regular transactions of reportMalicious? Are you sure?

[varasev]

If we're going to implement the above scheme of three addresses, maybe we could use two addresses instead, joining the mining and reporting addresses. So, we would use only mining and staking addresses.

In this case, we could take a value of nonce count from the code which calls commitHash/revealSecret/emitInitiateChange by mining address and use that nonce for calling reportMalicious if it is called on the same block at which commitHash/revealSecret/emitInitiateChange is called.

To guarantee that the transactions of commitHash/revealSecret/emitInitiateChange will succeed, we could replace reverts in these functions with the corresponding ifs and returns. With these always success transactions we could be sure that the nonce will be incremented as expected.

Besides that, we could allow the mining address to call only commitHash/revealSecret/emitInitiateChange/reportMalicious functions (using TxPermission.allowedTxTypes) to prevent using the mining address for other purposes.

The main questions here are:

• can we take that nonce count from the code which calls commitHash/revealSecret/emitInitiateChange and use it for calling reportMalicious? (if it happens at the same block)
• can the node keep count of the transactions it sent to call reportMalicious more than one time per block?

[afck]

Then, the malicious validator A won't be reported by their friend B.

I'm not worried about that scenario: If someone is malicious they might as well modify their node's source code so that the node never reports their friends. There's no point differentiating between the human and the software here: If a validator is malicious, we need to simply assume that their node does whatever they want it to do.
And of course if more than 50% of the validators are malicious, we have a problem anyway.

It seems that the regular transaction reportMalicious will anyway be called on the block #101 by the node C even when the C is not validator any more.

It depends on what you mean by "on the block #101": If C observes misbehavior of A in block #100, it will create, enqueue and send a transaction right when it processes block #100. However, that transaction might not make it into the next block; it might get processed only in block #103, or even later, depending on how full the transaction queues are, etc.

is it really possible in Parity code for those regular transactions of reportMalicious?

It is possible to pick any nonce for a transaction you make. After that, the usual rules apply, i.e. transactions can only be added to a block at a point where their nonce matches the sender's account nonce.
So it's definitely possible to keep count of the number of transactions we sent and to set the nonce accordingly. I'm not so sure, however, what to do in cases where a transaction is stalled and never gets added to a block: that would stall all the following ones, too.

calling reportMalicious if it is called on the same block at which commitHash/revealSecret/emitInitiateChange is called

The problem is: With the current code, we know exactly which block a transaction with commitHash, revealSecret or emitInitiateChange will be added to, because we always only include those in blocks that we author ourselves. The reportMalicious calls, however, go through the transaction queue and could be added at any later point in time. So if we used the same address, any of the calls to the former methods would automatically invalidate a pending call to reportMalicious. We could:

• Make the addresses separate and not merge them.
• Make reportMalicious calls the same way we make commitHash calls: but then we can only make them once we author a block again, which doesn't necessarily happen.
• Make the other calls the same way as reportMalicious again: then they can happen with any delay, possibly dozens of blocks after they were supposed to be called. (And we need to make even more sure we have a plan to deal with stalled transactions.)
• Make reportMalicious calls using both methods! I.e. in addition to keeping count of the calls that are in flight, and using different nonces for each of them, whenever we author a block, we'd make sure we add all our pending reportMalicious calls before the commitHash etc. call. That's complicated, but it might work.

So I guess my answer to both your questions is: "Yes, but terms and conditions apply."
It almost sounds like the last option would be the best. But implementation is going to be messy… 😬

[varasev]

It depends on what you mean by "on the block #101": If C observes misbehavior of A in block #100, it will create, enqueue and send a transaction right when it processes block #100.

Maybe then it is possible to call reportMalicious by system on the block #100 when misbehaviour is discovered? (using block header)

I saw that reportBenign is called by all nodes at the same time. So, the regular tx for reportMalicious should work the same way at the moment, I think.

Thus, when someone misbehaves, as far as I understand, all other nodes know about it and call report* function immediately. Maybe we could make reportMalicious callable by system as it happens with BlockReward.reward. Or even pass the info about malicious validator through extra parameters of BlockReward.reward?

With the current code, we know exactly which block a transaction with commitHash, revealSecret or emitInitiateChange will be added to, because we always only include those in blocks that we author ourselves. The reportMalicious calls, however, go through the transaction queue and could be added at any later point in time. So if we used the same address, any of the calls to the former methods would automatically invalidate a pending call to reportMalicious.

I agree, but I wrote that maybe we could take an incremented value of nonce count from the code which calls commitHash/revealSecret/emitInitiateChange by mining address and use that nonce for calling reportMalicious if it is called on the same block at which commitHash/revealSecret/emitInitiateChange is called.

Is it possible to take that incremented nonce and use it when calling reportMalicious?

• Make reportMalicious calls using both methods! I.e. in addition to keeping count of the calls that are in flight, and using different nonces for each of them, whenever we author a block, we'd make sure we add all our pending reportMalicious calls before the commitHash etc. call. That's complicated, but it might work.

Seems the same problem still exists for this method 👆 :

There can be a case when validator had reported but after that was removed from the set of validators (at the end of staking epoch). In that case, on_prepare_block (and thus reportMalicious) won't be called on the turn of the removed validator.

Or I missed something?

[afck]

Maybe then it is possible to call reportMalicious by system on the block #100 when misbehaviour is discovered? (using block header)

I'm not sure what you mean by that: If we discover misbehavior in block #100, that means block #100 has already been sealed, and probably has been authored by another node, so it's too late to put anything (header fields or transactions) into block #100.
Only for objectively detectable misbehavior (that doesn't depend e.g. on our local clock, our secret key, etc.), we could make that kind of implicit system call on block #100 itself, without adding any new information to it. But for one specific node accusing another of malice, we need to add some information to the blockchain.
In general, we'll only be able to put something in a block's header fields if we're the block author. If someone else authors the block, we always need to send them a message (that isn't guaranteed to arrive in time), e.g. with a transaction, and hope they include it.

when someone misbehaves, as far as I understand, all other nodes know about it

I agree, for that objective kind of misbehavior, we should be able to use system calls.

maybe we could take an incremented value of nonce

Yes, I think that's essentially the last bullet point in my comment above. It's just a bit difficult to track both the pending transactions and the other calls, and get the nonces right in all cases.

Seems the same problem still exists for this method

I don't think so: We'd also send our malice reports as transactions to other nodes, so even if we don't author another block, someone else would probably commit our transactions.

[varasev]

I'm not sure what you mean by that: If we discover misbehavior in block #100, that means block #100 has already been sealed, and probably has been authored by another node, so it's too late to put anything (header fields or transactions) into block #100.

Got it.

We'd also send our malice reports as transactions to other nodes, so even if we don't author another block, someone else would probably commit our transactions.

That would be great.

[afck]

Sorry for the back and forth, but I still see huge problems with the global nonce counter, especially if a validator is running more than one node.

I think it's more robust to keep a cache of malice reports, i.e. (validator, block_number) tuples, and retry creating transactions for them until they get committed, i.e. until ValidatorSetAuRa.maliceReportedForBlock returns true a value containing our own mining key in a finalized block.

But we'd need a similar getter for benign reports: If a contract doesn't use benign reports at all, benignReportedForBlock should always return true, so that the engine can discard its reports.

[varasev]

Sorry, but I don't understand what exactly changes you need to be implemented in contracts for reportMalicious?

At the moment, maliceReportedForBlock returns the list of validators which reported about malicious _validator for the _blockNumber.

Also, reportMalicious emits ReportedMalicious event if it could help: https://github.com/poanetwork/posdao-contracts/blob/7f0d8d04d44831bc5bbfeb685628c21edff308c2/contracts/ValidatorSetAuRa.sol#L103

[afck]

At the moment, maliceReportedForBlock returns the list of validators which reported about malicious _validator for the _blockNumber.

Right, sorry! Then what I should have said above is that we need to check whether that return value contains our own mining key, and if it does, we can remove our malice report from the cache.

Regarding benign reports: If we don't remove them entirely from the code, we should add a benignReportedForBlock method to the contracts that works analogously.

[varasev]

Regarding benign reports: If we don't remove them entirely from the code, we should add a benignReportedForBlock method to the contracts that works analogously.

Let's just comment out calling reportBenign in our Parity code but leave the logs about benign behavior as they are (so that we could see benign reports in node's log).

[varasev]

So firstly, we need to keep count of the number of malice reports that are in flight, to give them distinct nonces. Whenever we author a block, we should push all our own pending malice reports onto it before the commitHash/emitInitiateChange/… calls in on_prepare_block.

Is this still actual? (your very first comment 👆)

[varasev]

• Periodically re-send transactions reporting every entry in the list.

For this point, for each list entry we must firstly check if we already called reportMalicious for the given tuple (maliciousValidator, blockNumber) or not. If we did, we shouldn't call reportMalicious for that tuple again because this will increment the nonce and the next entry in the list won't be mined (because of nonce issue).

We could use maliceReportedForBlock for that checking: if our mining address is in returned array for the given tuple (maliciousValidator, blockNumber), we shouldn't call reportMalicious for that tuple.

[afck]

Is this still actual?

No, you're right. I updated it.

If we did, we shouldn't call reportMalicious for that tuple again because this will increment the nonce

I'd actually just resend all the pending reports periodically, and restart with the current account nonce. Yes, this will replace some of the other transactions that might be in flight, but we'd just keep retrying until they get through.

We could use maliceReportedForBlock for that checking

Yes, that's exactly what I'm proposing: Once that returns our own mining address, we can remove the tuple from the cache and don't need to call reportMalicious with it again.

[DemiMarie]

Is this something I can begin working on? Or is there something else I should be working on?

…

On Wed, Feb 13, 2019, 2:49 PM Andreas Fackler ***@***.*** wrote: Is this still actual? No, you're right. I updated it. If we did, we shouldn't call reportMalicious for that tuple again because this will increment the nonce I'd actually just resend *all* the pending reports periodically, and restart with the current account nonce. Yes, this will replace some of the other transactions that might be in flight, but we'd just keep retrying until they get through. We could use maliceReportedForBlock for that checking Yes, that's exactly what I'm proposing: Once that returns our own mining address, we can remove the tuple from the cache and don't need to call reportMalicious with it again. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGWB7XZ_BHMlyyq352UAf5jmKJ-wKlbks5vNGxLgaJpZM4a2mq4> .

[varasev]

Maybe I misunderstood this point:

• Whenever a block is finalized, remove every (validator, block_number) tuple from the list for which on that block maliceReportedForBlock(validator, block_number) contains our own mining key.

Does this happen in on_prepare_block? If it doesn't, then OK. Otherwise, we have the following case (correct me if I'm wrong):

Imagine that we have 2 tuples in the list and the current nonce is 10.

Let's assume that for the first tuple reportMalicious has already been called but on_prepare_block is not called yet because it wasn't our turn.

When you call reportMalicious for the first tuple again, this call fails (because of this requirement), but the nonce is incremented and becomes equal to 11.

When right after that you try to call reportMalicious for the second tuple, it fails because of the nonce issue.

[afck]

I think I'd actually call it in AuthorityRound::on_close_block (possibly only if some minimum amount of time has passed since the previous attempt), using the account nonce from after that block.

But I think what you're describing can always happen: We simply can't control how long it takes from the time where we create a transaction with the reportMalicious call until it gets actually added to a block, and by then its nonce could always be outdated.

That's why we'll just periodically retry: If one of the transactions fails to be committed, an equivalent transaction, with the same (validator, block_number) tuple but a new nonce, will be created in a later on_close_block call.

Also, in your example, we'd at some point call maliceReportedForBlock(v1, n1), find our own mining key in the result and remove (v1, n1) from the pending report cache. We'd then only create one more transaction with the second tuple (v2, n2), and with the current nonce.

[afck]

Is this something I can begin working on?

Yes, you can! 👍

[afck]

@DemiMarie: Feel free to start working on the report retry logic (the checkboxes at the end of the first comment). @varasev is already working on the contracts themselves, to separate mining and staking accounts.

[varasev]

I think I'd actually call it in AuthorityRound::on_close_block (possibly only if some minimum amount of time has passed since the previous attempt), using the account nonce from after that block.

Is on_close_block called on every block or only on our turn? It's important question because we can get stuck calling reportMalicious and can't call the second tuple successfully until on_close_block is called on our turn (and that way removes the first tuple from the list).

Let's review that situation I described above:

So, we have 2 tuples in the list and the current nonce is 10.

Let's assume that for the first tuple reportMalicious has already been successfully called but on_close_block is not called yet because it wasn't our turn. So, the first tuple is still in the list (along with the second tuple).

You can't call reportMalicious for the second tuple in the list, because the calling for the first tuple permanently increases the nonce every time we handle the list of tuples: when you call reportMalicious for the first tuple again, this call fails (because of this requirement), the nonce is incremented and becomes equal to 11.

When right after that you try to call reportMalicious for the second tuple, it fails because of the nonce issue.

Then, you handle the list again and call reportMalicious for the first tuple again. It fails again (because of the same requirement and again increases the nonce. When right after that you try to call reportMalicious for the second tuple, it fails again because of the nonce issue.

And this situation is repeated until on_close_block is called (to remove the first tuple).

That's why I suggest to use maliceReportedForBlock for checking of each entry of the list of tuples before calling reportMalicious when periodically handling the list.

[afck]

AuthorityRound::on_close_block is called on every turn, not just our own. But even if it's not our turn and another validator added our report to the block, we'd still remove the first tuple from the list.
(Well, that's another complication: It would be safer to only remove the tuple once the corresponding block has been finalized, i.e. has > N/3 confirmations…)

But I agree with your suggestion: Let's filter the list using maliceReportedForBlock every time we recreate our transactions. 👍
(That also works if there are committed but not yet finalized tuples; those would be filtered out and not retried, but if their block was actually reverted, they would be resent, as intended.)

[varasev]

Okay.

[varasev]

But I agree with your suggestion: Let's filter the list using maliceReportedForBlock every time we recreate our transactions.

Let's add this to the description in the very first comment above for the point Periodically re-send transactions reporting every entry in the list.

[afck]

@DemiMarie: I updated the description; let's reduce the number of redundant reports that way. ⬆️

[DemiMarie]

@afck Do we get a convenient callback when a block is finalized? If not, can we just check in on_close_block?

[afck]

I don't think there's a callback for that, and I doubt that on_close_block will be called again once a block gets finalized. We'll need to do some research to find out where this code should go.

I think it would be reasonable to remove the entries from the cache earlier (in on_close_block?) for now (with a big TODO!), and make the change with the finality in a separate PR, if it turns out to be difficult.

[afck]

@DemiMarie: Please test your changes with #101 when they're ready. Unfortunately that's a bit cumbersome because obviously we don't want to merge the afck-sim-malice branch, but at least it's a quick and dirty way to try out whether it works.

It would be best to even do two separate test runs in the end, I think: One with the Client::transact call disabled, to check whether committing in on_prepare_block works, and one with the malice reports in on_prepare_block disabled, to check that the Client::transact calls are made with valid transactions.

[varasev]

The fix of this issue was moved from #98 to #107.

[varasev]

Done in #129.