-
Notifications
You must be signed in to change notification settings - Fork 246
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
retry sending specific messages #4969
Conversation
Jenkins BuildsClick to see older builds (84)
|
2fbae93
to
c884f3c
Compare
d234dd0
to
f43babd
Compare
b17aa49
to
17ea9ed
Compare
thanks to @osmaczko , leave chat history with @osmaczko here as reference later cc @cammellos : Hi Patryk, maybe you know something about this: I'm struggling with
which use Relate PR: #4969 Relate code/comment: status-go/protocol/messenger_communities.go Line 1167 in 74ee7ef
status-go/protocol/messenger_communities.go Line 1173 in 74ee7ef
status-go/protocol/messenger_communities.go Line 1190 in 74ee7ef
status-go/protocol/messenger_communities.go Line 1344 in 74ee7ef
status-go/protocol/messenger_communities.go Line 1510 in 74ee7ef
Hey Frank,
About the related comment: "TODO(frank): need support resending following message? if yes then we need to ensure SendMessageToControlNode will invoke SendPrivate internally" If there are privileged members in the community, this should always be true: Btw. Why do we need to ensure Hi Patryk , thank you for your explaination!
Because when i was trying to implement the resending logical, considering what the Let's take following code as an example to discuss, status-go/protocol/messenger_communities.go Line 1167 in 74ee7ef
we can see that it used SendPrivate to send the raw message, and it reused the same variable rawMessage as param which means they will have the same value on rawMessage.ID, we have a field named Recipients for RawMessage which is used to determine who is the receiver(could be multi) of the raw message. If the same raw message always use SendPrivate to send, it's okay, but.. now think about this, we have a raw message that ID is 0x1, it could be sent with SendPrivate and SendCommunityMessage at the same time because SendMessageToControlNode could use SendPrivate or SendCommunityMessage ! What are we going to do with such case?
What I tried to say last time, is that, it should not happen with the current flow. If Let me explain more in deep: Right now, with owner-token concept, the owner of the community is the one who holds the token and we have a mechanism to deterministically obtain the chat key of that owner. Also, the private key of the community is gone once the owner-token is minted. That's why we Since we dropped import by private key, we could, in theory, use |
3865002
to
d487673
Compare
You may noticed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beast of a PR! Left some questions. And thanks a lot for very nice inline documentation.
protocol/migrations/sqlite/1712447150_update_raw_messages_with_resend_features.up.sql
Outdated
Show resolved
Hide resolved
d487673
to
3672759
Compare
When run
relate test code:
After checking code and
If you know something on this, do tell me, thank you! @vitvly cc @cammellos |
Solved by custom DiscV5BootstrapNode, if you have better solution, welcome! cc @richard-ramos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the best solution. As we discussed, having your own custom network (like you do here) removes the dependency on the fleet reducing the flakiness of this test.
Good work!
a238ba3
to
03a1800
Compare
03a1800
to
a3071bd
Compare
Unfortunately, the error still can be reproduced, but the frequency seems lower than before(6/100). @richard-ramos
|
I have run this a couple of times succesfully:
please share the logs for when there's a failure. I'm thinking that adding a 2s sleep in the test after initializing the nodes should help, since WakuRelay takes some time to form a mesh |
more specific and smaller range logs: |
fa3a38a
to
694e4a2
Compare
694e4a2
to
ca414a1
Compare
FYI, append these lines into
cc @richard-ramos @cammellos @vitvly @chaitanyaprem cheers! |
ca414a1
to
afbb823
Compare
func (b *GethStatusBackend) CreateAccountAndLogin(request *requests.CreateAccount) (*multiaccounts.Account, error) { | ||
// CreateAccountAndLogin creates a new account and logs in with it. | ||
// NOTE: requests.CreateAccount is used for public, params.Option maybe used for internal usage. | ||
func (b *GethStatusBackend) CreateAccountAndLogin(request *requests.CreateAccount, opts ...params.Option) (*multiaccounts.Account, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this changes expected here? I remember I added opts and later removed it.
Then this PR reintroduce it, and I don't see anywhere is using it. @qfrank
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kaichaosun , it's been used here
Relate mobile issue:
Major changes include:
ResendAutomatically
withResendType
andResendMethod
.rawMessageByID
function, which incorrectly calculatedRecipients
.