Fix Benchmark Test (BenchmarkReadMessage/Funding_Locked) in the lnwire package

[ziggie1984]

Change Description

This PR fixes #7152

The benchmark tests failed as described in the issue. Currently the tests produce random data for the ExtraOpaqueData field of most of the message types. This can be dangerous as soon as new data is added to a msg type via a tlv record. In this case other tests will fail too, if they use the random data generator for the tlv part.
The benchmark tests failed now, because the funding_locked msg type is the first message in the code base which has an tlv record nested in the overall message type therefore this behaviour was not observed before. Except the Open_channel msg but this is a special kind of message because the upfront_script has to be always in the tlv_data therefore the tests swap the ExtraOpaqueData and encode the relevant upfront_script record in the tlv stream

My approach was to narrow down the funding_locked message, discarding the random part of the ExtraOpaqueData data and including the new AliasScid tlv record, this way the tests will test the encode and decode method of the funding_locked msg more throughly.

Though as soon as another msg type introduces a new nested tlv field the tests will fail again. So maybe we should get rid of the random message generation part for the ExtraOpaqueData field for all msg types, or restrict its size to 0, and add additional tests which really try to push the max size of the whole message to its boundaries to actually test the size limits of the payload etc.

Moreover I added the Benchmark tests to the CI which which only runs the benchmark tests, explicitly avoiding all the other unit tests with the test.run=NONE option.

Steps to Test

In the lnwire directory run: go test -v -bench=BenchmarkReadMessage

or more general:

make unit-bench which runs all benchmark tests of all packages

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[morehouse]

The benchmark tests failed now, because the funding_locked msg type is the first message in the code base which has an tlv record nested in the overall message type therefore this behaviour was not observed before.

Don't we do something similar for UpfrontShutdownScript in OpenChannel and AcceptChannel? Why do the benchmarks pass for them?

Moreover should we also include the Benchmark tests in the overall testrun sothat we capture future failure more reliably?

Yes, I think this is the best way to avoid bit rot. Then when a new TLV is introduced, the test will fail and alert us that we need to update the benchmarks.

[ziggie1984]

Thanks for your review, appreciate it :)

Good point, thats because the upfrontscript needs to be in the tlv-data, thats a hard requirement according to the bolts:

if both nodes advertised the option_upfront_shutdown_script feature:
MUST include upfront_shutdown_script with either a valid shutdown_scriptpubkey as required by shutdown scriptpubkey, or a zero-length shutdown_scriptpubkey (ie. 0x0000).

https://github.com/lightning/bolts/blob/master/02-peer-protocol.md

therefore the compelete random string of ExtraOpaqueData gets swapped for the new 0x0000 byte tlv record. Thats why when reading this tlv record everything is fine and no random data is in the Opaque field. For the funding_locked there is no mandatory field in the tlv record therefore the random stream is written to the ExtraOpaqueData

The swapping happens here

[ziggie1984]

Because its a benchmark test, maybe its better to write a function which fills the ExtraOpaqueData data with valid tlv records (sorted) and also consider each msg for itself, adding as much data as possible until max data is reached for every message?

Otherwise just passing almost empty ExtraOpaqueData to the parser, as now done by the funding_locked msg (the current change) is not really worth it for a benchmark

[morehouse]

Thanks for the fix!

One suggestion below on simplifying the new code.

[morehouse]

I think we could simplify this code a bit. Something like:

msg := lnwire.FundingLocked{
  ChanID: lnwire.ChannelID(c),
  NextPerCommitmentPoint: pubkey,
  AliasScid: lnwire.NewShortChanIDFromInt(r.Int63()),
  ExtraData: make([]byte, 0),
}

The RecordProducer stuff below to populate ExtraData could stay the same.

[ziggie1984]

Done, Though I had to keep the aliasScid separate because the AliasScid is a pointer but the NewShortChanIDFromInt returns a value type

[morehouse]

LGTM

[morehouse]

IIUC, we don't need to call EncodeMessageExtraData here, since it will also be done inside Encode.

[ziggie1984]

Thanks you are right, its now part because we write it before 👍

[ziggie1984]

Though I think also including it in the extradata is the correct way of doing it, because normally when calling newMsgFundingLocked I would expect a correctly filled message of this type without having to know that I need to encode it first so the TLV data is included in the right place ?

[ziggie1984]

but I am good with both ways, I deleted it but added a remark why we do not write the TLV data in the ExtraData in the method. I think that's the best way.

[guggero]

What's the test.run=XXX here? Is the intent to only run very short benchmark tests to make sure they work in general but finish quickly?

[ziggie1984]

My intention with the XXX regex was to only run the bench-mark tests without any other tests, because otherwise it would run the normal unit-tests too. And this make commd should only handle the Benchmark Tests ?

I will add a comment clarifying it, and also changed the regex to something better with NONE which is a special value in golang and matches no tests

[ziggie1984]

Hmm I understand your points regarding adding it to the CI tests, so the initial issue of this PR was, because it was not on anyone's radar that the Benchmark tests were failing. But adding them to the CI is also maybe not the right approach. Maybe only execute them for specific releases so that we make sure they do not fail and pass at least ?

[guggero]

Hmm I understand your points regarding adding it to the CI tests, so the initial issue of this PR was, because it was not on anyone's radar that the Benchmark tests were failing. But adding them to the CI is also maybe not the right approach. Maybe only execute them for specific releases so that we make sure they do not fail and pass at least ?

Perhaps we can just run the benchmark tests on master branch? So they would be run after each merged PR but at least not for each update of each PR, which already saves some cycles but would still be noticed when they break (when we actually have fully green CI which is hopefully very soon).
What I mean is: if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/master' }} in the unit-bench job.

[ziggie1984]

I like your idea so I changed the CI workflow for Benchmark tests.

[yyforyongyu]

Thanks for the PR! I like that we are considering putting benchmarks into our CI, but the current approach may not add many benefits.

At the moment, for a particular PR, we run benchmarks twice, one before and one after, to assess the performance improvement brought by the PR. Running benchmarks alone, however, doesn't provide us with any useful info.

I thought about adding a new workflow here to run benchmarks periodically. Or run them per release to see the performance change between releases, or run them for particular PRs, with the help of a github bot to report the results.

So I'd say we should remove it from CI because it doesn't give us useful info. And similar bugs can get fixed whenever the benchmarks are run. Not ideal, until we can properly figure out how to run them automatically.

[ziggie1984]

At the moment, for a particular PR, we run benchmarks twice, one before and one after, to assess the performance improvement brought by the PR. Running benchmarks alone, however, doesn't provide us with any useful info.

What do you mean by "run twice" here, are the benchmarks already included in some kind of test-script or "should" a developer run the benchmark tests twice?

Anyways I agree maybe just put them out of the CI for now as you said and look in a separate PR how we can integrate the benchmarks properly.

[yyforyongyu]

LGTM👍

What do you mean by "run twice" here, are the benchmarks already included in some kind of test-script or "should" a developer run the benchmark tests twice?

I meant the developer should run it twice to understand the performance change. An example would be #4884, which used the bench results to guide the code change.

[lightninglabs-deploy]

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

[yyforyongyu]

Maybe this can be rebased and merged🧐

[ziggie1984]

Maybe this can be rebased and merged🧐

Rebased and ready to be merged I think.

[guggero]

LGTM 🎉