chore: do not mount lightpush without relay (fixes #2808) #3540
Conversation
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
LGTM! Thanks for it! 💯
NagyZoltanPeter
left a comment
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
I think it is mostly ok, but I have some concerns.
First of all legacy lightpush shall be treated the same as v3.
On the other hand I think we need to adapt also here: https://github.com/waku-org/nwaku/blob/644b5928c757c07202aec67bc8fc1a11730b374d/waku/factory/conf_builder/waku_conf_builder.nim#L592-L598
Due otherwise the node will advertise lightpush capability while it does not has it.
Finally there was a discussion around local first approach... which is not implemented for lightpush yet, but we had some conclusion I don't remember exactly the outcome.
Meaning that possible to use lightpush without valid relaying might still reach filter and thus local testing is possible from edge node POV.
I think this shall not prevent this PR for now, just for the record.
cc: @Ivansete-status
|
Thanks @NagyZoltanPeter , I have a question about this:
I assumed that calling it "legacy" implies we are not making new changes to it. Is this not the case? Are we aiming to develop two versions of Lightpush in parallel, what is the rationale there? |
Sure, unlikely to expect changes in legacy lightpush protocol. But the rationale with this treatment that from clients pov it is mainly one service. Of course there are clients supports only old one... hopefully for not tooo long, the rest can support both for now. So it is not very good that a service node will unexpetedly support only the old protocol while v3 is not mounted due to the circumstances of not having relay. Meanwhile, I know this is a very, very rare case. |
Ivansete-status
force-pushed
the
master
branch
from
5118fc3 to
08d14fb
Compare
|
You can find the image built from this PR at Built from b6e4414 |
|
Thanks for this comment @NagyZoltanPeter |
- Change mountLightPush signature to return Result[void, string] - Return error when relay is not mounted - Update all call sites to handle Result return type - Add test verifying mounting fails without relay - Only advertise lightpush capability when relay is enabled
s-tikhomirov
force-pushed
the
chore/do-not-mount-lightpush-without-relay
branch
from
4079d07 to
0930cb9
Compare
|
Rebased and addressed the comments above. The logic now applies to both lightpush and legacy lightpush. I see that interop and js-waku tests are failing (in addition to macos-15 and ubuntu-22.04 builds that seem to be failing regardless for instance on my other PR as well). I'm unsure what to do about it... |
s-tikhomirov
force-pushed
the
chore/do-not-mount-lightpush-without-relay
branch
from
7b695a2 to
a6d231a
Compare
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
LGTM! Thanks for it! 🙌
|
Thanks for recent changes @s-tikhomirov |
NagyZoltanPeter
left a comment
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you!
|
@s-tikhomirov Can you please check if failing js-waku tests are because of these changes? |
|
You can merge the PR as it seems the source of the breaking change |
|
folks, I changed my mind: |
client side of protocols are always mounted.. .so that means edge mode if relay not mounted but client req/resp protocols. |
Description
Previously, when mounting Lightpush, if Relay was not mounted, a nil Lightpush protocol handler was used. This PR changes node setup logic: mounting Lightpush without Relay mounted results in an error. The rationale behind this change is that a node cannot serve a Lightpush request without Relay. Fixes #2808.
Changes
Result-type value)Issue
closes #2808