Fd 1264 factable december #945
Conversation
receiver names, small refactors, unit test additions.
refactors.
previous PR. 1) Change to iota, 2) change >=
a duplicate function of existing nextAuth2() already in the testing suite
of nextAuth2() after removing other duplicates
to reference *RCD_1, so that caching can work. b) remove zeroReader1 which is a copy of existing zeroReader already in the library, c) Add new unit test for RCD which checks RCD 3 times. - a corrupted sig - a real sig - another corrupted sig This test passes BEFORE my changes because the caching mechanism doesn't work. After my changes the caching mechanism is proven to work, which causes the test to FAIL because the cached result of the 'good' signature returns 'good' even when a corrupted signature is presented to the RCD.
|
This is good stuff Michael. Thank you. I am glad you found some off-by-one errors. That shows an exceptional attention to detail. I am seeing some errors on the CI runs. if you click on the red x you can see what has gone wrong. https://circleci.com/gh/FactomProject/factomd/16362#tests/containers/1 Sometimes CircleCI tests are flaky, and rerunning them can get them to pass, but this doesn't look like one of those issues. feel free to add more commits to the end of your branch and it should update this pull request and rerun circleCI. if it starts flaking, you can also make minor commits to trigger it again. also make sure you run keep them coming. This is good stuff. we are mostly going on vacation for the next couple weeks, so we won't be on the next two Thursday developer standups, but early 2020 we will be back. |
|
Hi Brian, yes, the factoid test suite will fail because it contains my new unit test which fails after I enabled the caching mechanism to work. Because the rcd structure will now verify a bad signature if it has previous verified a good signature. I specifically kept it failing so people would notice this, and if we all agree to enable the caching with this limitation, then I'd suggest I comment out the lines of the failing test with a note why they are commented out, and then everything should be ok. |
|
That can work. I haven't dived too deep into the tradeoffs of the issue, so I don't know if it is something that can be fixed easily. Do you want to try to fix the underlying problem? Maybe you can add a cache freshness flag or something. Perhaps reading the cache would verify what the cache was based on is still the same? I am assuming here, but my guess is the expensive operation is the signature checking. You would have a better idea how to fix it. We have setup github to not allow us to pull in code with failing tests. Either way, we can't adopt the pull request as it is. Do whichever of them makes more sense to you. |
comment on why they fail, and in the future when we should re enable them
|
I removed the failing lines of code with a comment. My only concern with fixing it right now, is I don't have a good grasp of the implications of changing a datastructure that currently exists in the block chain. For instance, if I change either the RCD or the transaction structs, does that impact data in the block chain when you search for it and pull it into the struct via json or some other probing? I'm worried that I'll break something in my ignorance. :-) Normally data structures are just internal and you can move them around to your liking, but I believe with the blockchain they actually interface with external things that are reliant on a certain structure. But maybe i'm wrong |
| Booting RunState = iota // State when starting up the server | ||
| Running RunState = iota // State when doing processing | ||
| Stopping RunState = iota // State when shutdown has been called but not finished | ||
| Stopped RunState = iota // State when shutdown has been completed |
There was a problem hiding this comment.
You do not need to place iota on each line.
There was a problem hiding this comment.
Reviewed, agree with @Emyrk here. Rest looks good.
There was a problem hiding this comment.
Fixed and checked in. Thanks!
Variety of changes. Lots of comments, receiver changes, minor refactors, some additions to unit tests. Two changes to pay attention to, are the runstates.go where some real code changes were made. And to rcd1.go where a new unit test was created. It grabs a transaction, the rcd, and its signature. Then corrupts the signature and tries to verify the RCD fails (which it does with current code), then tries to verify with the proper signature to make sure it passes (which it does with current code), and then tries to verify the RCD again with a corrupted signature to make sure it fails (which it does with current code). This test passes with current code. My changes fix a problem in the RCD1 caching mechanism where it was not caching the verification correctly. Not it caches it correctly, however, this causes the unit test i just created to now FAIL, because the second RCD check with the corrupted signature comes AFTER a 'good' check which sets the cache to be true, and so any verification after will be reported as true. I was told that this wouldn't be an issue because in the code you can't use the RCD to verify on a different signature. But just look over this section carefully.