Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[custom channels]: refactor channel JSON, add more custom data parsers #1441

Merged
merged 6 commits into from
Apr 1, 2025
Merged

[custom channels]: refactor channel JSON, add more custom data parsers #1441

merged 6 commits into from
Apr 1, 2025

Conversation

guggero
Copy link
Member

@guggero guggero commented Mar 13, 2025 •
edited
Loading

Depends on lightningnetwork/lnd#9504.

Corresponding litd PR: lightninglabs/lightning-terminal#1017

Contains some commits from #1413, will rebase once this one is in.

Improves the structure of the asset channel related JSON data for multi-asset (group key) support.

@coveralls
Copy link

coveralls commented Mar 13, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 14198379200

Details

  • 90 of 191 (47.12%) changed or added relevant lines in 5 files are covered.
  • 41 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.2%) to 28.379%
Changes Missing Coverage Covered Lines Changed/Added Lines %
rpcserver.go 0 2 0.0%
rfq/manager.go 0 5 0.0%
tapchannelmsg/custom_channel_data.go 79 173 45.66%
Files with Coverage Reduction New Missed Lines %
rpcserver.go 1 0.0%
address/mock.go 2 86.93%
asset/group_key.go 2 57.89%
commitment/tap.go 2 71.82%
tapchannel/aux_leaf_signer.go 3 43.08%
tapchannelmsg/custom_channel_data.go 3 56.1%
tapdb/universe.go 4 80.57%
tapchannel/aux_invoice_manager.go 5 83.13%
asset/mock.go 8 64.29%
asset/asset.go 11 50.3%
Totals Coverage Status
Change from base Build 14195018952: 0.2%
Covered Lines: 25856
Relevant Lines: 91111
💛 - Coveralls

@levmi levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Mar 24, 2025
@guggero guggero force-pushed the more-custom-channel-data branch from df1bbbe to b236594 Compare March 27, 2025 17:42
@guggero guggero mentioned this pull request Mar 27, 2025
Merged
@guggero guggero requested a review from GeorgeTsagk March 27, 2025 17:46
@guggero guggero force-pushed the more-custom-channel-data branch 2 times, most recently from 85ec0c5 to 8170bcd Compare March 27, 2025 19:32
@guggero guggero marked this pull request as ready for review March 27, 2025 19:33
@guggero guggero requested a review from gijswijs March 27, 2025 19:34
gijswijs
gijswijs approved these changes Mar 28, 2025
View reviewed changes
Copy link
Contributor

@gijswijs gijswijs left a comment

Choose a reason for hiding this comment

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

tACK. Why does the itest fail in CI? Locally everything runs fine.

tapchannelmsg/records_test.go
// Proofs don't Encode everything, so we need to do a quick Encode/
// Decode cycle to make sure we can compare it afterward.
originalRandProof := randProof(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice refactor.

@guggero
Copy link
Member Author

guggero commented Mar 28, 2025

tACK. Why does the itest fail in CI? Locally everything runs fine.

Looks like a flake. Or perhaps we need to increase the itest timeout, since they now seem to be running a bit longer.

@guggero guggero force-pushed the more-custom-channel-data branch from 8170bcd to 5e8b0c4 Compare March 31, 2025 15:00
@guggero
Copy link
Member Author

guggero commented Mar 31, 2025

Hmm, this is a new flake: unable to fetch set of universe servers: unknown sqlite error: unable to open database file: out of memory (14)

Very interesting. Added another commit to attempt to clean up some space on the GitHub runner.

GeorgeTsagk
GeorgeTsagk approved these changes Mar 31, 2025
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

Lgtm 🍏
pending itest flake

@github-project-automation github-project-automation bot moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Mar 31, 2025
@Roasbeef
Copy link
Member

Roasbeef commented Apr 1, 2025 

--- FAIL: TestBookAssetSyncer (0.32s)
    test_sqlite.go:11: Creating new SQLite DB handle for testing: /tmp/TestBookAssetSyncer352891606/001/tmp.db
    custodian_test.go:591: 
        	Error Trace:	/home/runner/work/taproot-assets/taproot-assets/tapgarden/custodian_test.go:591
        	Error:      	Received unexpected error:
        	            	unable to make address for unknown asset 9efe64d9cd4c73561967aedc06c08800b61d3913ed5d55e6a135e7e1f7fbd69c: asset group is unknown
        	Test:       	TestBookAssetSyncer

@guggero guggero mentioned this pull request Apr 1, 2025
Merged
guggero added 6 commits April 1, 2025 14:01
@guggero
tapchannelmsg: refactor and unit test JSON formatting
a6f7e2b 
This commit refactors the JSON formatting of the channel related custom
data fields from lnd and unit tests the JSON output.
@guggero
tapchannelmsg: improve channel state JSON structure
bb81abd 
The previous structure barely made sense with a single asset UTXO, but
is completely wrong with multiple asset UTXO committed to a single
channel.
We fix the structure and also add more data from the encoded blob to it.
@guggero
mod: bump to latest lnd version
2e9bf9b
@guggero
tapchannelmsg: parse missing channel data
7296703
@guggero
GitHub: point to updated litd branch
91636c9
@guggero
GitHub: make some space before running large CI steps
9d8d52f 
We're apparently running out of space in our integration tests at some
point: `unable to fetch set of universe servers: unknown sqlite error:
unable to open database file: out of memory (14)`
The GitHub action runners aren't really outfitted with a lot of storage,
but apparently deleting the hosted tools directory can free up some
space.
@guggero guggero force-pushed the more-custom-channel-data branch from 5e8b0c4 to 9d8d52f Compare April 1, 2025 14:31
@guggero
Copy link
Member Author

guggero commented Apr 1, 2025 

--- FAIL: TestBookAssetSyncer (0.32s)
    test_sqlite.go:11: Creating new SQLite DB handle for testing: /tmp/TestBookAssetSyncer352891606/001/tmp.db
    custodian_test.go:591: 
        	Error Trace:	/home/runner/work/taproot-assets/taproot-assets/tapgarden/custodian_test.go:591
        	Error:      	Received unexpected error:
        	            	unable to make address for unknown asset 9efe64d9cd4c73561967aedc06c08800b61d3913ed5d55e6a135e7e1f7fbd69c: asset group is unknown
        	Test:       	TestBookAssetSyncer

I ran the flake hunter for quite a while on the rebased version of this and wasn't able to trigger it.
So perhaps the disk space cleanup also fixed this? Got green CI at first attempt after rebase, so I'm going to merge.

@guggero guggero merged commit a9ea76a into main Apr 1, 2025
18 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Taproot-Assets Project Board Apr 1, 2025
@guggero guggero deleted the more-custom-channel-data branch April 1, 2025 15:05
@guggero guggero mentioned this pull request Apr 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gijswijs gijswijs gijswijs approved these changes

@GeorgeTsagk GeorgeTsagk GeorgeTsagk approved these changes

Assignees

@guggero guggero

Labels
P1 tap-channels
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@guggero @coveralls @Roasbeef @gijswijs @GeorgeTsagk @levmi