Skip to content

Throw error for RGS data that's more than two weeks old #1921

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.

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

Merged

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Dec 16, 2022

Fixes #1785

@arik-so arik-so force-pushed the 2022-12-prohibit-old-rgs-updates branch from 630e041 to 9550a6f Compare December 16, 2022 20:44
@@ -16,6 +16,9 @@ use lightning::io;
use crate::error::GraphSyncError;
use crate::RapidGossipSync;

#[cfg(all(feature = "std", not(test), not(feature = "_test_utils")))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not _test_utils?

@TheBlueMatt
Copy link
Collaborator

Note, CI is failing here.

@arik-so
Copy link
Contributor Author

arik-so commented Dec 18, 2022

Lol I know. That's why I added the constraints beyond std, a test was complaining about an unused import. Still iterating.

@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2022

Codecov Report

Base: 87.21% // Head: 87.22% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (f33e0c8) compared to base (30b9d9f).
Patch coverage: 92.30% of modified lines in pull request are covered.

❗ Current head f33e0c8 differs from pull request most recent head 2f36c92. Consider uploading reports for the commit 2f36c92 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1921      +/-   ##
==========================================
+ Coverage   87.21%   87.22%   +0.01%     
==========================================
  Files         100      100              
  Lines       44023    44058      +35     
  Branches    44023    44058      +35     
==========================================
+ Hits        38396    38431      +35     
  Misses       5627     5627              
Impacted Files Coverage Δ
lightning-rapid-gossip-sync/src/lib.rs 78.26% <50.00%> (-2.34%) ⬇️
lightning-rapid-gossip-sync/src/processing.rs 93.33% <97.05%> (+0.57%) ⬆️
lightning-background-processor/src/lib.rs 81.67% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.29% <0.00%> (+0.01%) ⬆️
lightning/src/util/events.rs 24.78% <0.00%> (+0.57%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator

Any reason this is still draft? Can you drop the _test_utils feature added?

@arik-so
Copy link
Contributor Author

arik-so commented Dec 21, 2022

Yeah, because I'm still experimenting with how to do the very thing you asked for.

I haven't been able to remove test utils yet because the background processor test for pruning needs to feed old data to RGS without it erroring, but the test config flag doesn't carry over to a different crate.

I'm wondering if I could modify the test to be just shy of two weeks old, or perhaps stub out std time somehow.

@TheBlueMatt
Copy link
Collaborator

Ah, sorry. Can the test not be rewritten to use the mock time (which could be advanced to move beyond today)?

@arik-so
Copy link
Contributor Author

arik-so commented Jan 3, 2023

The test could use mock time, but the code that's being tested uses std time. Or are you saying that the mocker can override std time's response values in a test environment? Are there example tests that do that?

@TheBlueMatt
Copy link
Collaborator

I think you could set the mock to std time, then walk the time forward using the mock and it should work?

@arik-so arik-so force-pushed the 2022-12-prohibit-old-rgs-updates branch from d29451e to a5271ea Compare February 9, 2023 17:39
@arik-so arik-so changed the title WIP: Throw error for RGS data that's more than two weeks old Throw error for RGS data that's more than two weeks old Feb 9, 2023
@arik-so arik-so marked this pull request as ready for review February 9, 2023 17:40
@arik-so
Copy link
Contributor Author

arik-so commented Feb 9, 2023

I am dedraftifying this PR.

@arik-so arik-so force-pushed the 2022-12-prohibit-old-rgs-updates branch from a5271ea to 2f65b87 Compare February 9, 2023 17:43
@arik-so arik-so requested a review from jkczyz February 9, 2023 19:00
@arik-so arik-so force-pushed the 2022-12-prohibit-old-rgs-updates branch from 2f65b87 to 7fd91af Compare February 9, 2023 21:43
Comment on lines +48 to +49
// Note that many tests rely on being able to set arbitrarily old timestamps, thus we
// disable this check during tests!
Copy link
Contributor

Choose a reason for hiding this comment

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

As of now, the test inputs are a bit mysterious. If we programmatically construct them rather than use fixed values, shouldn't we be able to enable this in testing (and make the test more understandable)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. See my other comment :)

@@ -526,6 +557,115 @@ mod tests {
assert!(after.contains("783241506229452801"));
}

#[test]
fn full_update_succeeds_with_edge_timestamp_age() {
const LATEST_SEEN_TIMESTAMP: u64 = 1642291930;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the value chosen here significant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. It is the timestamp encoded in the input bytes.

Comment on lines 563 to 579
let valid_input = vec![
76, 68, 75, 1, 111, 226, 140, 10, 182, 241, 179, 114, 193, 166, 162, 70, 174, 99, 247,
79, 147, 30, 131, 101, 225, 90, 8, 156, 104, 214, 25, 0, 0, 0, 0, 0, 97, 227, 98, 218,
0, 0, 0, 4, 2, 22, 7, 207, 206, 25, 164, 197, 231, 230, 231, 56, 102, 61, 250, 251,
187, 172, 38, 46, 79, 247, 108, 44, 155, 48, 219, 238, 252, 53, 192, 6, 67, 2, 36, 125,
157, 176, 223, 175, 234, 116, 94, 248, 201, 225, 97, 235, 50, 47, 115, 172, 63, 136,
88, 216, 115, 11, 111, 217, 114, 84, 116, 124, 231, 107, 2, 158, 1, 242, 121, 152, 106,
204, 131, 186, 35, 93, 70, 216, 10, 237, 224, 183, 89, 95, 65, 3, 83, 185, 58, 138,
181, 64, 187, 103, 127, 68, 50, 2, 201, 19, 17, 138, 136, 149, 185, 226, 156, 137, 175,
110, 32, 237, 0, 217, 90, 31, 100, 228, 149, 46, 219, 175, 168, 77, 4, 143, 38, 128,
76, 97, 0, 0, 0, 2, 0, 0, 255, 8, 153, 192, 0, 2, 27, 0, 0, 0, 1, 0, 0, 255, 2, 68,
226, 0, 6, 11, 0, 1, 2, 3, 0, 0, 0, 4, 0, 40, 0, 0, 0, 0, 0, 0, 3, 232, 0, 0, 3, 232,
0, 0, 0, 1, 0, 0, 0, 0, 29, 129, 25, 192, 255, 8, 153, 192, 0, 2, 27, 0, 0, 60, 0, 0,
0, 0, 0, 0, 0, 1, 0, 0, 0, 100, 0, 0, 2, 224, 0, 0, 0, 0, 58, 85, 116, 216, 0, 29, 0,
0, 0, 1, 0, 0, 0, 125, 0, 0, 0, 0, 58, 85, 116, 216, 255, 2, 68, 226, 0, 6, 11, 0, 1,
0, 0, 1,
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there instructions anywhere on how this was produced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, it was just copied from the other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been meaning to create a valid RGS generator for a while now. Perhaps it's time, though I think probably best in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Could you open an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just did: #2030

@jkczyz
Copy link
Contributor

jkczyz commented Feb 14, 2023

I think you could set the mock to std time, then walk the time forward using the mock and it should work?

This may work, but you'd have to change the time parameter from Option<u64> to Option<Duration> and use Time::duration_since_epoch, I believe.

@jkczyz
Copy link
Contributor

jkczyz commented Feb 14, 2023

LGTM other than the possible use of util::Time.

TheBlueMatt
TheBlueMatt previously approved these changes Feb 15, 2023
@@ -29,10 +32,30 @@ const GOSSIP_PREFIX: [u8; 4] = [76, 68, 75, 1];
/// avoid malicious updates being able to trigger excessive memory allocation.
const MAX_INITIAL_NODE_ID_VECTOR_CAPACITY: u32 = 50_000;

/// We remove disallow gossip data that's more than two weeks old, per BOLT 7's
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/remove//?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oof, yup

#[test]
fn full_update_fails_with_old_timestamp() {
const LATEST_SEEN_TIMESTAMP: u64 = 1642291930;
let valid_input = vec![
Copy link
Collaborator

Choose a reason for hiding this comment

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

it'd be nice to make these all one constant, eg by running the whole thing in one test instead of 3, rather than having to check that manually.

@TheBlueMatt
Copy link
Collaborator

Yea, could use util::Time, but I don't think it matters for an internal-only thing, its certainly simpler to not use it here :)

@arik-so
Copy link
Contributor Author

arik-so commented Feb 15, 2023

Sorry guys, was a bit busy with other stuff, but the reason I chose u64 instead of time is because of this other place where do it:

fn channel_failed_with_time(&self, short_channel_id: u64, is_permanent: bool, current_time_unix: Option<u64>) {

Now, if you ran git blame, you'd notice that it was actually me who added it, but I promise that I believe that I remember copying that from some other instance in our codebase 🙃

@arik-so arik-so force-pushed the 2022-12-prohibit-old-rgs-updates branch 2 times, most recently from f33e0c8 to e0b4a80 Compare February 16, 2023 07:30
@TheBlueMatt
Copy link
Collaborator

There's still two separate tests with the same input, right? Feel free to squash the fixup.

@arik-so arik-so force-pushed the 2022-12-prohibit-old-rgs-updates branch from e0b4a80 to 2f36c92 Compare February 16, 2023 20:17
@TheBlueMatt TheBlueMatt merged commit 4b043c9 into lightningdevkit:main Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RGS Accepts Stale Data Without Error
4 participants