Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge bitcoindevkit#1752: Remove serde json dependency from chain crate
1c81cd5 refactor(chain)!: change String by &str for versioned scripts in migrate_schema (nymius) 265b59b test(chain): add compatibility test for v0 to v1 sqlite schema migration (nymius) 4ec7940 test(wallet): pattern match ChainPosition::Confirmed in anchors persistence check (nymius) 2eb19f5 refactor(chain)!: move schemas to their own methods (nymius) 5603e9f test(chain): use ChangeSet<ConfirmationBlockTime> instead of ChangeSet<BlockId> (nymius) c3ea54d test(wallet): check persisted anchors does not lose data (nymius) d41cb62 build(chain): remove serde_json dependency (nymius) 7319f51 refactor(chain)!: remove AnchorImpl wrapper for Anchor implementors (nymius) b587b0f refactor(chain)!: impl sqlite for ConfirmationBlockTime anchored changesets (nymius) Pull request description: ### Description As expressed by @LLFourn _We want to not depend on serde_json. If we keep it around for serializing anchors we won't be able to remove it in the future because it will always be needed to do migrations_ Currently there is only one widely used anchor, `ConfirmationBlockTime`. The decision was to constrain support to just be for a single anchor type ConfirmationBlockTime. The anchor table will represent all fields of `ConfirmationBlockTime`, each one in its own column. The reasons: - No one is using rusqlite with any other anchor type, and if they are, they can do something custom anyway. - The anchor representation may change in the future, supporting for multiple Anchor types here will cause more problems for migration later on. Resolves bitcoindevkit#1695 ### Notes to the reviewers <details> <summary>Why the type of the confirmation_time column is INTEGER?</summary> By [sqlite3 docs](https://www.sqlite.org/datatype3.html): > Each value stored in an SQLite database (or manipulated by the database engine) has one of the following storage classes: > ... > INTEGER. The value is a *signed integer*, stored in 0, 1, 2, 3, 4, 6, or 8 bytes depending on the magnitude of the value. (Remember confirmation time is `u64`) > REAL. The value is a floating point value, stored as an 8-byte IEEE floating point number. > ... > SQLite uses a more general dynamic type system. In SQLite, the datatype of a value is associated with the value itself, not with its container. > ... > In order to maximize compatibility between SQLite and other database engines, ..., SQLite supports the concept of "type affinity" on columns. The type affinity of a column is the recommended type for data stored in that column. The important idea here is that the type is recommended, not required. Any column can still store any type of data. It is just that some columns, given the choice, will prefer to use one storage class over another. The preferred storage class for a column is called its "affinity". > ... > A column with NUMERIC affinity may contain values using all five storage classes. When text data is inserted into a NUMERIC column, **the storage class of the text is converted to INTEGER or REAL (in order of preference)** if the text is a well-formed integer or real literal, respectively. > A column that uses INTEGER affinity behaves the same as a column with NUMERIC affinity. But anchor table was defined using the STRICT keyword, again, by sqlite docs: > ... The STRICT keyword causes the following differences: > ... > SQLite attempts to coerce the data into the appropriate type using the usual affinity rules, as PostgreSQL, MySQL, SQL Server, and Oracle all do. *If the value cannot be losslessly converted* in the specified datatype, then an SQLITE_CONSTRAINT_DATATYPE error is raised. So, the *TLDR*, with some help of this [blog post](https://jakegoulding.com/blog/2011/02/06/sqlite-64-bit-integers/) is: - SQLite INTEGER supported values range from `-2**63 to (2**63-1)` - If the number is bigger it will treat the number as REAL. - As the SQLite table is defined with the STRICT keyword, it should enforce a losslessly conversion or fail with SQLITE_CONSTRAINT_DATATYPE. </details> <details> <summary>Why not setting confirmation_time as BLOB or NUMERIC?</summary> I don't have a strong opinion on this. INTEGER was the first numeric type I found, then later I found NUMERIC and they seemed to behave in the same way, so I didn't change the type. I discarded BLOB and ANY first because they didn't provide a clear idea of what was being stored in the column, but in retrospective they seem to remove all the considerations that I had to do above, so maybe they are better fitted for the type. </details> <details> <summary>Why adding a default value to confirmation_time column if the anchor column constraint is to be NOT NULL so all copied values will be filled?</summary> `confirmation_time` should have the same constraint as `anchor`, to be `NOT NULL`, but as UPDATE statements might be executed in already filled tables, you must provide a default value for all the rows you are going to add the new column to. As the `confirmation_time` extraction of the json blob in anchor cannot be performed in the same step, I had to add this default value. This is flexibilizing the schema of the tables and extending the bug surface it may have, but I'm assuming the application layer will enforce the addition of a valid `confirmation_time` always. </details> <details> <summary>Why the default value of confirmation_time column is -1?</summary> Considering the other alternatives were to use the max value, min value or zero and confirmation time should always be positive, I considered `-1` just to be computer and human readable enough to perceive there must be something wrong if the `ConfirmationBlockTime` retrieved by the load of the wallet has this value set as the confirmation time. </details> <details> <summary>Why to not be STRICT with each statement?</summary> It is a constraint only applicable to tables on creation. </details> <details> <summary>Why not creating a whole new table without anchor column and with the confirmation_time column, copy the content from one to the other and drop the former table?</summary> Computation cost. I didn't benchmark it, and I don't know how efficient is SQLite engine under the hood, but at first sight it seems copying a single column is better than copying four. </details> <!-- In this section you can include notes directed to the reviewers, like explaining why some parts of the PR were done in a specific way --> ### Changelog notice - Reduce `rusqlite` implementation only to `ChangeSet<ConfirmationBlockTime>`. - Replace `anchor` column in sqlite by `confirmation_time`. - Modify `migrate_schema` `versioned_script` parameter's type to `&[&str]`. - Transformed existing schemas in methods to allow their individual application. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added tests for the new feature * [ ] I've added docs for the new feature #### Bugfixes: * [x] This pull request breaks the existing API * [ ] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: evanlinjin: ACK 1c81cd5 notmandatory: ACK 1c81cd5 Tree-SHA512: 96facb59d49c29bb19096b0426c0d0c9a909a17541502b5a6f80b0075a333a354cfe9d0e247f07dd5931793412512ecdd80f22ae8ace85d0bb2cbb1abdfcfa9a
- Loading branch information