build: update to sqlite version 1.19.1 #126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were on a version a few months old. Main driver of this was I ran into this issue in another branch: https://gitlab.com/cznic/sqlite/-/issues/77. Updating fixes it, but then runs into some other related race. The author of the library asked about the first on the sqlite forums and that the race also exists in the orginal C codebase, but it isn't an issue. The tests fail as is, since there was a bug fix w.r.t how the NULL constraints and NULL insertion was handleed. It looks like this was resolved here: https://gitlab.com/cznic/sqlite/-/issues/100 and here https://gitlab.com/cznic/sqlite/-/issues/98. Seems like the classic case of `nil` vs `[]byte{}`.
The new DeepEqual methods added won't work properly. For example, in the TestImportAssetProof test, we use the randomly generated asset as the root asset in a split commitment. We serialize this as a raw asset within the split commitment proof. However, we have certain fields we maintain in memory in the struct that are never serialized. An example of such a field is the raw family key. As a result, these two assets will never return true from `DeepEqual`. For now, we go back to asserting against the serialized versions.
The update to sqlite fixes an issue where a nil byte slice would be stored/interpreted as a `[]byte` instead of a zero sized blob. As a result, after a restart, we would be passing down a NULL Taro root to the DB. With the older version this would be written, but the newer version now properly rejects this as a NULL constraint violation. We now also cache the script root and return that along side the key.
guggero
reviewed
Sep 20, 2022
| // not equal. | ||
| if !isEqual { | ||
| require.Equal(t, witB, witA) | ||
| require.Equal(t, witA.PrevID, witB.PrevID) |
There was a problem hiding this comment.
Oops, sorry about that! And thanks for the fix 🙏
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were on a version a few months old.
Main driver of this was I ran into this issue in another branch:
https://gitlab.com/cznic/sqlite/-/issues/77.
Updating fixes it, but then runs into some other related race. The
author of the library asked about the first on the sqlite forums and
that the race also exists in the orginal C codebase, but it isn't an
issue.
The tests fail as is, since there was a bug fix w.r.t how the NULL
constraints and NULL insertion was handleed. It looks like this was
resolved here: https://gitlab.com/cznic/sqlite/-/issues/100 and here
https://gitlab.com/cznic/sqlite/-/issues/98. Seems like the classic case
of
nilvs[]byte{}.For now, I've removed the constraint on the witness stack, since today
we rely on the behavior where we can read out a NULL element for
the witness of a genesis asset.