Skip to content

lightningd: Default value for received_time in listforwards to 0#7744

Merged
vincenzopalazzo merged 1 commit intoElementsProject:masterfrom
s373nZ:7157-listforwards-received-time-default
Oct 26, 2024
Merged

lightningd: Default value for received_time in listforwards to 0#7744
vincenzopalazzo merged 1 commit intoElementsProject:masterfrom
s373nZ:7157-listforwards-received-time-default

Conversation

@s373nZ
Copy link
Contributor

@s373nZ s373nZ commented Oct 16, 2024 •
edited
Loading

This pull request fixes #7157 by altering the JsonRpc return value of listforwards to always return a value for the received_time field, initializing it to 0 if not found. These values could be missing as a result of forwarding attempts that were created prior the implementation of tracking this data, ostensibly in v0.7.0 (?).

This also removes the COMPAT_V070 compiler directive logic for forwards.c. Since received_time is required by the JSON schema, the old backward compatibility strategy is breaking and no longer viable.

Alternative implementation approaches

I wasn't 100% sure which layer to address this issue in. Some alternative implementation ideas were:

  1. Add a default key to the received_time field in the listforwards response definition in the JSON schema here. This didn't produce any change to cln-rpc/src/model.rs, so the assumption is this is unsupported.
  2. Remove received_time as a required field in the response schema here. Decided against this because it felt like more of a breaking change to the API from a client perspective.
  3. Create a database migration altering the forwards table setting received_time to 0. (see comment below)
  4. Remove the conditional code in forwards.c processed by the COMPAT_V070 compiler directive. The compatibility changes are turned on but they are incompatible with the current schema's field requirement for received_time. Need to ask out under which conditions compatibility can be removed. <--- ended up doing this one :)

Questions

  1. Can the backwards compatibility for listforwards be removed? <--- ended up doing this :)
  2. Doubtful, but is there a way to use conditionals which test for previous COMPAT_V* versions in the command schemas, lightning-listfowards.json with the idea of conditionally requiring the field?
  3. Can we consider COMPAT_V070 be considered fully deprecated, @cdecker mentioned on Discord it's generally 6 months, but this seems ancient?

Important

Open for merging new PRs until the next PR freeze date is confirmed!

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

@s373nZ s373nZ changed the title forwards: Default value for received_time in listforwards` to 0 forwards: Default value for received_time in listforwards to 0 Oct 16, 2024
@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch 2 times, most recently from 850d836 to 6171cf3 Compare October 16, 2024 14:46
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 16, 2024

Reintroduced the COMPAT_V070 compiler directive. These changes should really only apply without compatibility mode. Also added a short note about the default in the listforwards documentation.

@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch 2 times, most recently from 7660e48 to 7e095bf Compare October 17, 2024 09:08
@s373nZ s373nZ changed the title forwards: Default value for received_time in listforwards to 0 lightningd: Default value for received_time in listforwards to 0 Oct 17, 2024
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 17, 2024

After some historical research, it seems the database migration setting received_time to 0 was already performed in #3191 which was meant to address #3189. So, alternative implementation 3 above probably isn't relevant.

@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 17, 2024

Discovered that the COMPAT_V* flags are actually enabled for the build by default, so the code inside the COMPAT_V070 compiler directive is actually what is important here. When that flag is removed from the build instruction, the original code works. Added removing the compiler directive to alternative options list as 4.

@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch 3 times, most recently from b9f89da to 4e8d077 Compare October 17, 2024 16:07
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 17, 2024

Removed the COMPAT_V070 compiler directive code only for forwards.c. I think there is a case to be made that the old compatibility code is downright broken for these old forward records when received_time is required by the command schema.

@s373nZ s373nZ marked this pull request as ready for review October 17, 2024 16:15
@s373nZ s373nZ requested a review from cdecker as a code owner October 17, 2024 16:15
@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch from 4e8d077 to 97782c6 Compare October 18, 2024 10:35
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 18, 2024

After more testing, I removed my initial changes to set a default 0 for received_time in code as well. All the old forwards should be initialized to 0 in this migration and testing indicates that if NULL somehow snuck into the database, it breaks at the database layer, when trying to parse the result.

The changeset now just reflect ripping out the COMPAT_V070 changes for forwards.c without any other functional changes.

@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch 2 times, most recently from cfe5bb8 to 8708051 Compare October 22, 2024 18:01
@s373nZ
Copy link
Contributor Author

s373nZ commented Oct 22, 2024

Modified the test to stop & start the l2 node between updating the database record, and skipped the test if TEST_DB_PROVIDER doesn't indicate sqlite3.

@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch from 8708051 to 81650bf Compare October 25, 2024 15:38
@s373nZ
lightningd: listforwards returns 0 for missing received_time. ([E…
98c2f26 
…lementsProject#7157])

Removes the `COMPAT_V070` functionality for `listfowards`.

Changelog-Changed: The `listforwards` command will now return a value
of 0 for `received_time` for very old forward attempts.
@s373nZ s373nZ force-pushed the 7157-listforwards-received-time-default branch from 81650bf to 98c2f26 Compare October 25, 2024 19:16
@rustyrussell
Copy link
Contributor

rustyrussell commented Oct 26, 2024

Reasonable solution, thanks!

rustyrussell
rustyrussell approved these changes Oct 26, 2024
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 98c2f26

vincenzopalazzo
vincenzopalazzo approved these changes Oct 26, 2024
View reviewed changes
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 98c2f26

@vincenzopalazzo vincenzopalazzo merged commit 4017844 into ElementsProject:master Oct 26, 2024
@s373nZ s373nZ deleted the 7157-listforwards-received-time-default branch October 26, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rustyrussell rustyrussell rustyrussell approved these changes

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

+1 more reviewer

@vincenzopalazzo vincenzopalazzo vincenzopalazzo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cln-rpc: missing field received_time

3 participants

@s373nZ @rustyrussell @vincenzopalazzo