feat(transaction/validate): advance transaction validation when producing blocks #5582
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #5582 +/- ##
=============================================
+ Coverage 66.43% 66.50% +0.07%
- Complexity 10216 10260 +44
=============================================
Files 894 902 +8
Lines 53841 53884 +43
Branches 5931 5935 +4
=============================================
+ Hits 35768 35835 +67
+ Misses 15284 15264 -20
+ Partials 2789 2785 -4 ☔ View full report in Codecov by Sentry. |
| protected Pair<GrpcAPI.Return.response_code, String> doValidate(TransactionCapsule trx) { | ||
| if (trx.getSerializedSize() > Constant.TRANSACTION_MAX_BYTE_SIZE) { | ||
| return buildResponse(GrpcAPI.Return.response_code.TOO_BIG_TRANSACTION_ERROR, | ||
| "too big transaction, the size is %d bytes", trx.getSerializedSize()); |
There was a problem hiding this comment.
- Why change the logic of getting transaction size?
Too big transactionpreviously. Is it necessary to keep all exception messages consistent in case they are useful for some Dapps?
There was a problem hiding this comment.
- check size don't need to ask for new arrays.
- in fact, it returns like that.
There was a problem hiding this comment.
In fact, the modification of the return value will not have much impact, but it cannot rule out all situations, such as log filtering and other monitoring systems.
This is just a suggestion, after all, in my opinion, the amount of work required to maintain a consistent or new approach is the same.
| @Override | ||
| protected Pair<GrpcAPI.Return.response_code, String> doValidate(TransactionCapsule trx) { | ||
| byte[] transactionId = trx.getTransactionId().getBytes(); | ||
| if (transactionCache.has(transactionId) && transactionStore.has(transactionId)) { |
There was a problem hiding this comment.
transaction cache uses Bloom filters, which have a false positive rate when found.
There was a problem hiding this comment.
halibobo1205
force-pushed
the
feat/advance_trans_check_for_pack
branch
from
ac7e087 to
0b6bdca
Compare
| int contractSize = trx.getContractSize(); | ||
| if (contractSize != 1) { | ||
| return buildResponse(GrpcAPI.Return.response_code.CONTRACT_VALIDATE_ERROR, | ||
| "contract size should be exactly 1, this is extend feature ,actual :%d", |
There was a problem hiding this comment.
Former msg format contains: tx %s contract size should be exactly 1
| protected Pair<GrpcAPI.Return.response_code, String> doValidate(TransactionCapsule trx) { | ||
| byte[] transactionId = trx.getTransactionId().getBytes(); | ||
| if (transactionCache.has(transactionId) && transactionStore.has(transactionId)) { | ||
| return buildResponse(GrpcAPI.Return.response_code.DUP_TRANSACTION_ERROR, "dup trans"); |
There was a problem hiding this comment.
Former msg is: 'dup trans : %s '
| trx.validateSignature(accountStore, dynamicPropertiesStore); | ||
| return SUCCESS; | ||
| } catch (ValidateSignatureException e) { | ||
| return buildResponse(GrpcAPI.Return.response_code.SIGERROR, e.getMessage()); |
There was a problem hiding this comment.
Former is " %s transaction signature validate failed", txId"
There was a problem hiding this comment.
trx.validateSignature(AccountStore accountStore, DynamicPropertiesStore dynamicPropertiesStore) never returns false, the following are not valid.
halibobo1205
force-pushed
the
feat/advance_trans_check_for_pack
branch
from
0b6bdca to
2987803
Compare
| import org.tron.api.GrpcAPI.Return.response_code; | ||
| import org.tron.core.capsule.TransactionCapsule; | ||
|
|
||
| public abstract class AbstractTransactionValidator implements Validator< |
There was a problem hiding this comment.
Is this abstract design really needed? It seems to validate some simple and fixed things only but with so many classes imported.
| import org.tron.core.capsule.TransactionCapsule; | ||
|
|
||
| public abstract class AbstractTransactionValidator implements Validator< | ||
| Pair<response_code, String>, TransactionCapsule> { |
There was a problem hiding this comment.
The name response_code shall be changed to fit java code style.
There was a problem hiding this comment.
response_code is generated by proto.
| @@ -1597,6 +1534,10 @@ public BlockCapsule generateBlock(Miner miner, long blockTime, long timeout) { | |||
| postponedTrxCount++; | |||
| continue; // try pack more small trx | |||
| } | |||
| // check transaction | |||
| if (!transactionValidator.silentValidate(trx)) { | |||
There was a problem hiding this comment.
Actually, some checks may be skipped if that has been checked when it was pushed into pending queue.
There was a problem hiding this comment.
Not necessary for now.
|
There is no significant performance improvement, closed for now. |
close #5564.