[sql-56] session: fix FeatureConfigs with an empty config set in kvdb to sql migration

[ViktorT-11]

In the bbolt store, an empty config for a feature is represented as an empty array, while in for the SQL store, the same config is represented as nil. Therefore, in the scenario where a specific feature has an empty config, we override the SQL FeatureConfig for that feature to also be set to an empty array. This is needed to ensure that the deep equals check in the migration validation does not fail in this scenario.

[gemini-code-assist]

Summary of Changes

Hello @ViktorT-11, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue in the session migration process from bbolt to SQL where reflect.DeepEqual validation would incorrectly flag discrepancies for feature configurations. The core problem stemmed from bbolt representing empty feature configs as empty arrays, while the SQL store would represent them as nil. A new override mechanism has been implemented to normalize these empty configurations to an empty array in the SQL session during migration, ensuring accurate validation and a consistent data state.

Highlights

• Feature Config Normalization: Introduced a new function overrideFeatureConfig to standardize the representation of empty feature configurations during session migration. This addresses a discrepancy where bbolt stores empty configs as empty arrays, while SQL stores them as nil.
• Migration Validation Fix: The overrideFeatureConfig ensures that reflect.DeepEqual checks in the migration validation do not fail due to the nil vs. empty array difference for feature configurations, by overriding the SQL representation to an empty array when the bbolt version was also empty.
• Test Coverage Enhancement: Added new test cases to session/sql_migration_test.go to specifically cover scenarios involving sessions with empty and nil feature configurations, and modified randomFeatureConfig to generate empty configs for more robust testing.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a discrepancy in how empty feature configurations are represented between the bbolt and SQL stores, which was causing migration validation failures. The introduction of the overrideFeatureConfig function to normalize the SQL representation to a non-nil empty slice is a sound approach. The changes are well-contained and accompanied by appropriate tests, including specific cases for empty and nil configs, and updates to the randomized test data. The overall implementation is correct and effectively solves the issue.

[bitromortac]

Another approach would be to convert nil configs to empty ones in unmarshalFeatureConfigs. Wouldn't that be better because that way we ensure that both session (kvdb/sql) retrievals are consistent?

[ViktorT-11]

We discussed this offline, and we decided to continue with the override approach in favour of changing the unmarshal function. While that approach will likely work, we'll keep the same approach we've used to tackle similar issues previously, and consider to in the future instead change to the unmarshal approach. But when doing so, we'll change for all current overrides, and not just this case.

[bitromortac]

From a call-site perspective I think it's fine to keep the nil here

lightning-terminal/session_rpcserver.go

Lines 1550 to 1555 in 5e22106

	clientConfig := make(map[string]string)
	if sess.FeatureConfig != nil {
	for k, v := range *sess.FeatureConfig {
	clientConfig[k] = string(v)
	}
	}

This just leads to an empty string.

[lightninglabs-deploy]

@ViktorT-11, remember to re-request review from reviewers when ready

[bitromortac]

Should this be an &&? Otherwise it could be that mSession.FeatureConfig is nil, which would panic below.

[bitromortac]

What if we erroneously migrated an empty feature config to a non-empty one, then we'd silently override the migrated one, right? So maybe we could just convert the migrated config if it is nil to an empty one?

[bitromortac]

From a call-site perspective I think it's fine to keep the nil here

lightning-terminal/session_rpcserver.go

Lines 1550 to 1555 in 5e22106

	clientConfig := make(map[string]string)
	if sess.FeatureConfig != nil {
	for k, v := range *sess.FeatureConfig {
	clientConfig[k] = string(v)
	}
	}

This just leads to an empty string.