feat: improving unit tests for Basic BLS and enforcing Basic BLS for Evo Nodes#5463
Conversation
ogabrielides
left a comment
There was a problem hiding this comment.
Could add the same check on CProRegTx.
UdjinM6
left a comment
There was a problem hiding this comment.
we should really have similar checks in corresponding IsTriviallyValids too
We indeed need to have this validation, I added them in PR. Btw, there's no unit tests for it because: I created issue for it: #5471 |
## Issue being fixed or feature implemented It partially resolves issue #5471 Better unit tests are needed to validate changes in ProTx implementation such as this PR: #5463 ## What was done? - Invalid ProTx transactions are checked more strictly. The flag "tx is failed" is not enough now for test to succeed, but error code should matched with expected error. - Duplicated implementations of tests for "valid" and "invalid transaction" are changed to more general code. - Added extra log output with tx ID for easier debug - to see which exactly tx is failed in test - Supported more by 256 txes in one json file ## How Has This Been Tested? Run unit tests ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone
|
This pull request has conflicts, please rebase. |
1ef2b6f to
11add29
Compare
|
@PastaPastaPasta Reasons for keeping this one open? |
|
It seems like we're not gonna merge this for v20 right? I think that's better but that's correct right? |
I intentionally changed milestone to v21 - we have already RC; let's don't increase scope for v20. |
|
Hmm... I would actually suggest to merge it into v20 - it streamlines the logic and extends unit tests a lot. Imo it a valuable one. |
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK for squash merge
| // unknown version, bail out early | ||
| return; | ||
| } | ||
|
|
Issue being fixed or feature implemented
#5471
What was done?
It splits from #5443
Adds extra unit tests for BLS basic scheme; enforces BLS basic for Evo Nodes in serialization/unserialization of CProRegTx, CProUpServTx.
How Has This Been Tested?
Run functional/unit tests + added new unit tests.
Breaking Changes
Serialization slightly changed, but it should be not breaking change
Checklist: