breez / breez-sdk-greenlight

MIT License
244 stars 43 forks source link

Persist `SendPay.groupid` as `TEXT` to cover full `u64` range #1124

Closed ok300 closed 5 days ago

ok300 commented 1 week ago

Restoring a node kept panicking with a ToSqlConversionFailure(TryFromIntError(())) error. Turns out this was caused by how we persist SendPay.groupid.

In cln_grpc this field is defined as uint64 and deserialized as u64:

https://github.com/ElementsProject/lightning/blob/e66653fa1d847e28bd812356a762225a44bfae83/cln-grpc/proto/node.proto#L935

However we map it to an INTEGER in the DB, which actually accepts signed 64 bit numbers. If we try to persist positive integers that use the 64th bit, inserting it into SQL will fail with ToSqlConversionFailure(TryFromIntError(())).

This is exactly what caused restore to fail in my case, as I had several groupids bigger than 2^63.

This PR fixes this by converting SendPay.groupid to String and storing it as TEXT. Since we only use it as a String, we don't have to re-convert it back to u64 when deserializing SendPay from the DB.

JssDWt commented 1 week ago

This PR fixes this by converting SendPay.groupid to String and storing it as TEXT. Since we only use it as a String, we don't have to re-convert it back to u64 when deserializing SendPay from the DB.

If I understand correctly, you had invalid groupids stored in your send_pays table. If we create the new table and insert everything from the old table, I think the groupids migrated from the old table would still be invalid? Potentially we won't identify multiple parts belonging to the same group correctly during sync because of it.

I think it's best to

The sdk will then resync everything.

ok300 commented 1 week ago

If I understand correctly, you had invalid groupids stored in your send_pays table.

No, started the CLI with a fresh data directory, so restore only pulled fresh new data.

I think it's best to

  • drop the send_pays table and recreate it with the new schema
  • drop the sync state by removing the sync_state entry from the cache

The sdk will then resync everything.

Done: 7c16a500c01f39518353e3d2067d9ce99e7b03c9

ok300 commented 1 week ago

Done.

JssDWt commented 1 week ago

Tested this on an existing node, and it works. Thinking about whether we should also clear the payments table as a precaution for potentially invalid updates. It might not be necessary, but it won't do any harm either. What do you think @ok300?

roeierez commented 6 days ago

Tested this on an existing node, and it works. Thinking about whether we should also clear the payments table as a precaution for potentially invalid updates. It might not be necessary, but it won't do any harm either. What do you think @ok300?

I think it is not necessary because syncing will replace invalid payments.