Update sigtype along with the rest of account data #783
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #783 +/- ##
===========================================
+ Coverage 54.46% 54.65% +0.19%
===========================================
Files 28 28
Lines 3909 3930 +21
===========================================
+ Hits 2129 2148 +19
- Misses 1496 1497 +1
- Partials 284 285 +1
Continue to review full report at Codecov.
|
tolikzinovyev
force-pushed
the
tolik/sigtype-optimization
branch
from
3ec1762
to
5af2d40
Compare
| @@ -364,7 +364,30 @@ func (w *Writer) addTransactionParticipation(block *bookkeeping.Block) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func writeAccountData(round basics.Round, address basics.Address, accountData basics.AccountData, batch *pgx.Batch) { | |||
| type optionalSigType struct { | |||
There was a problem hiding this comment.
This type isn't needed, a map[Address]SigType can be used. On a rekey you could delete(res, payset[i].Txn.Sender), the present check would change to compare sigtype to the zero value (or check if it is in the map).
There was a problem hiding this comment.
I think that wouldn't work. We need to distinguish between three operations: leave sigtype as is, clear it, or set to something non-null. If an address is not in map[basics.Address]optionalSigType, we leave sigtype as is. If it is in the map and the value is optionalSigType with present = false, we clear it. Otherwise, we set sigtype to optionalSigType.value.
There was a problem hiding this comment.
You aren't using the 3rd case, this code sets something for each sender in the payset, even if it doesn't change.
There was a problem hiding this comment.
you mean the first case?
| return res, nil | ||
| } | ||
|
|
||
| func writeAccount(round basics.Round, address basics.Address, accountData basics.AccountData, sigTypeDelta optionalSigType, batch *pgx.Batch) { |
| return res, nil | ||
| } | ||
|
|
||
| type optionalSigTypeDelta struct { |
There was a problem hiding this comment.
This nested optional implementation is really confusing. Even knowing the problem it solves I still had a lot of trouble following how it works.
I think the code would be easier to understand with a single optional type (the one named sigTypeDelta), and a check for value.IsZero(). That way you either set the value to zero, or skip setting it.
is more complicated than the writeAccounts function you replaced with it. I think it would be better to have a single optional and check value.IsZero() for
There was a problem hiding this comment.
The reason why I did it this way is because idb.SigType doesn't have a special zero value. I think it shouldn't. I will add a comment for sigTypeDelta.
| @@ -379,6 +379,8 @@ func (w *Writer) addTransactionParticipation(block *bookkeeping.Block) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // Describes a change to the `account.keytype` column. If `present` is true, | |||
| // `value` is the new value. Otherwise, NULL will be the new value. | |||
There was a problem hiding this comment.
Thanks. This is still pretty confusing, but the comment helps. We probably should have added a UKNOWN SigType, but this works.
tolikzinovyev
force-pushed
the
tolik/sigtype-optimization
branch
from
701a05b
to
1e21104
Compare
Summary
Before we were updating the
account.keytypecolumn separately from updating accounts which resulted in one extra account lookup and one extra account rewrite by the database. This PR changes the way sigtype is updated; now it's done with the rest of account changes. The TPS improvement is 1.6X in my tests.Test Plan
We already have tests for the keytype, but I added another one that tests that even when an account is deleted after a round, the keytype is updated anyway.
One concern I had is that the evaluator might skip an account delta when the account data doesn't change even when it is a transaction sender (which triggers keytype change), but we already have a test (
TestKeytypeBasic) that uses zero fee transactions that don't modify the account state.