fix: Verify after sign not executing#1638
Conversation
CodSpeed Performance ReportMerging this PR will degrade performance by 39.4%Comparing Summary
Performance Changes
Footnotes
|
There was a problem hiding this comment.
Are there settings changes already in main that change the auto-add behavior of actions?
Because I would expect we don't need set_intent everywhere yet (this is a big breaking change). If we say the setting is gone it's fine, but we should be clear that it is gone then.
|
The changes in tests is partially due to a default behavior/settings change from #1586. Settings change + this should be accordingly clearly and very obviously flagged as breaking changes once this releases. This will break downstream SDKs, and their tests (at least for Python i am 100% sure there will be breakages). |
|
(I think you will still need to look into the benchmarks though, they seem quite unhappy right now). |
|
@tmathern That's expected if we are doing validation after signing. Maybe we should consider disabling validation after signing for those benchmarks since we are more concerned about the signing performance there? |
I'm just pointing out they seem unhappy and we can't merge like that. How you fix is dealer's choice. Can we have benchmarks with and without validation? So we'll also see the impact of the fix, and can track that in the future if we make more changes and it varies. Also I assume most people would have validate-after-sign on (isn't it going to be default behavior?), so not having benchmarks may be a bit misleading regarding perf. Middleground is to ahve data for both cases. |
gpeacock
left a comment
There was a problem hiding this comment.
This is a good catch, if we were not validating, then the updated tests were already producing invalid claims
Verify after sign is currently never ran regardless of the
verify.verify_after_signsetting.There exists a check to ensure we do not verify an empty stream (such as when
io::emptyis supplied), it involves ensuring the stream length is non-zero. However, in the current logic we rewind the stream beforehand resulting in the length always being set to 0. The fix is to seek to the end of the stream to properly read the length.