feat: allow ignoring PublishError.Duplicate #404

maschad · 2023-02-14T21:42:38Z

Closes #402

When ignoreDuplicatePublishError is set, instead of throwing with PublishError.Duplicate, simply return.

src/index.ts

codecov-commenter · 2023-02-15T20:17:33Z

Codecov Report

Base: 83.41% // Head: 83.31% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (5906a27) compared to base (41740b5).
Patch coverage: 76.69% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #404      +/-   ##
==========================================
- Coverage   83.41%   83.31%   -0.11%     
==========================================
  Files          48       48              
  Lines       11766    11800      +34     
  Branches     1266     1271       +5     
==========================================
+ Hits         9815     9831      +16     
- Misses       1951     1969      +18

Impacted Files	Coverage Δ
src/metrics.ts	`21.25% <0.00%> (-0.51%)`	⬇️
src/index.ts	`70.46% <80.00%> (+0.02%)`	⬆️
src/score/peer-score-params.ts	`98.21% <91.42%> (+0.75%)`	⬆️
src/score/peer-score-thresholds.ts	`100.00% <100.00%> (ø)`
src/types.ts	`94.57% <100.00%> (+0.16%)`	⬆️
test/gossip.spec.ts	`96.62% <100.00%> (+0.26%)`	⬆️

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.

…inSafe#402)

maschad · 2023-02-15T22:38:03Z

Looks like @dapplion had raised a similar issue #272

He also suggested that we could add an array of topics to the signature, what do you think @tuyennhv ?

src/metrics.ts

twoeths · 2023-02-16T01:30:47Z

Looks like @dapplion had raised a similar issue #272

He also suggested that we could add an array of topics to the signature, what do you think @tuyennhv ?

I'm not clear about the context in that issue, if we publish same message to multiple topics, we also need to modify the fastMsgIdFn for the received side. Also we can just call publish() multiple times with same message and different topics.

@dapplion do we need an array of topics or just an option to ignore duplicate check is enough?

what-the-diff · 2023-02-16T13:43:11Z

Added a new option to ignore duplicate messages
Updated the metrics for ignoring duplicates

…t are ignored (ChainSafe#402)

maschad · 2023-02-16T15:26:55Z

Thanks for reviewing @tuyennhv I have made the requested updates. I think we could probably address #272 in a separate PR since the issue was raised in a different context.

src/index.ts

src/types.ts

src/index.ts

src/metrics.ts

src/types.ts

wemeetagain · 2023-02-20T02:39:29Z

Added a new option to ignore duplicate messages

Updated the metrics for ignoring duplicates

This isn't right. -1 for the AI
Messages aren't being ignored. The metric isn't tracking ignored duplicates.
~~This feature allows us to publish duplicate messages. The metric tracks the number of published duplicates.~~
Edit: This feature allows us to avoid throwing an error when publishing a duplicate message. The metric tracks the number of duplicate message publish attempts.

wemeetagain · 2023-02-20T02:42:30Z

src/index.ts

    if (this.seenCache.has(msgIdStr)) {
      // This message has already been seen. We don't re-publish messages that have already
      // been published on the network.
+      if (allowPublishDuplicateMessages) {
+        this.metrics?.onPublishDuplicateMsg(topic)
+        return { recipients: [] }


@tuyennhv correct me if i'm wrong, this isn't what we want?
I thought we want to actually follow the same flow and actually send messages to peers?

in this case some other nodes already published this same message and it's around in the topic peers already, I found it's not helpful to send to topic peers again - some nodes already saw that message and sent it to us, they may already had it in their seen cache too.

In lodestar's scenario, builder published the block, then the node receives the message and try to publish again and it throws error. The published block was spread to the network successfully, only our api throws error.

maschad · 2023-02-20T18:18:21Z

Added a new option to ignore duplicate messages

Updated the metrics for ignoring duplicates

This isn't right. -1 for the AI Messages aren't being ignored. The metric isn't tracking ignored duplicates. ~~This feature allows us to publish duplicate messages. The metric tracks the number of published duplicates.~~ Edit: This feature allows us to avoid throwing an error when publishing a duplicate message. The metric tracks the number of duplicate message publish attempts.

Technically it was correct because we where returning early / an empty message which was the original intention in #402 i.e. option to publish api to return early and ignore the error

As @tuyennhv mentioned it's not helpful to republish to those peers again since some nodes already would've seen that message, thus they may have already had it in their seen cache too.

So with that in mind I posit that ignoreDuplicateMessages may be a more appropriate name as opposed to allowPublishDuplicateMessages

wemeetagain · 2023-02-20T20:40:17Z

Reason I'm not a fan of "ignoreDuplicateMessages" naming is that its too general and the terms feel overloaded.
We "ignore" messages in gossip validation, though this feature has nothing to do with this kind of ignoring. We also have two message flows (publish and forward) that handle duplicate messages, so "ignoreDuplicateMessages" sounds like a blanket statement about how we handle duplicate messages.

"allowPublishDuplicateMessage" follows the naming of the other "suppress a publish error" option - "allowPublishToZeroPeers" - generally "allowPublishErrorname".
Do agree there's some confusion in this naming as well tho.

Imo the core feature here is suppressing / avoiding a publish error. Probably there's some other name that can clearly articulate this.

maschad · 2023-02-20T23:09:58Z

Understood. I think ignoreDuplicatePublishError captures the essence of conveying that we are trying to avoid a publish error then.

feat: allow for duplicate messages (ChainSafe#402)

f7f59cd

maschad requested a review from a team as a code owner February 14, 2023 21:42

maschad changed the title ~~feat: allow for duplicate messages (#402)~~ feat: allow for duplicate messages Feb 14, 2023

twoeths reviewed Feb 15, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

feat: return empty array if ignoreDuplicateMessages is enabled (Chain…

64780ed

…Safe#402)

feat: added gauge metric for tracking ignored duplicate messages (Cha…

6c8c0c2

…inSafe#402)

maschad requested a review from twoeths February 15, 2023 20:20

twoeths reviewed Feb 16, 2023

View reviewed changes

src/metrics.ts Outdated Show resolved Hide resolved

twoeths reviewed Feb 16, 2023

View reviewed changes

src/metrics.ts Outdated Show resolved Hide resolved

feat: updated metric name for gauge to include published messages tha…

2d30405

…t are ignored (ChainSafe#402)

maschad requested a review from twoeths February 16, 2023 15:27

maschad mentioned this pull request Feb 16, 2023

Implement whatthediff #403

Closed

dapplion previously approved these changes Feb 19, 2023

View reviewed changes

twoeths previously approved these changes Feb 20, 2023

View reviewed changes

wemeetagain changed the title ~~feat: allow for duplicate messages~~ feat: allow publish of duplicate messages Feb 20, 2023

wemeetagain requested changes Feb 20, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

src/types.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

src/metrics.ts Outdated Show resolved Hide resolved

chore: renaming

68c897b

wemeetagain dismissed stale reviews from twoeths and dapplion via 68c897b February 20, 2023 02:33

wemeetagain reviewed Feb 20, 2023

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

chore: fix linter error

4b74381

wemeetagain requested changes Feb 20, 2023

View reviewed changes

maschad requested a review from wemeetagain February 20, 2023 18:18

chore: updated name to ignoreDuplicatePublishError (ChainSafe#402)

5906a27

wemeetagain approved these changes Feb 20, 2023

View reviewed changes

wemeetagain changed the title ~~feat: allow publish of duplicate messages~~ feat: allow ignoring PublishError.Duplicate Feb 20, 2023

wemeetagain merged commit dcde3c9 into ChainSafe:master Feb 21, 2023

github-actions bot mentioned this pull request Feb 21, 2023

chore(master): release 6.2.0 #401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow ignoring PublishError.Duplicate #404

feat: allow ignoring PublishError.Duplicate #404

maschad commented Feb 14, 2023 •

edited by wemeetagain

Loading

codecov-commenter commented Feb 15, 2023 •

edited

Loading

maschad commented Feb 15, 2023

twoeths commented Feb 16, 2023 •

edited

Loading

what-the-diff bot commented Feb 16, 2023

maschad commented Feb 16, 2023

wemeetagain commented Feb 20, 2023 •

edited

Loading

wemeetagain Feb 20, 2023

twoeths Feb 20, 2023

maschad commented Feb 20, 2023

wemeetagain commented Feb 20, 2023

maschad commented Feb 20, 2023

feat: allow ignoring PublishError.Duplicate #404

feat: allow ignoring PublishError.Duplicate #404

Conversation

maschad commented Feb 14, 2023 • edited by wemeetagain Loading

codecov-commenter commented Feb 15, 2023 • edited Loading

Codecov Report

maschad commented Feb 15, 2023

twoeths commented Feb 16, 2023 • edited Loading

what-the-diff bot commented Feb 16, 2023

maschad commented Feb 16, 2023

wemeetagain commented Feb 20, 2023 • edited Loading

wemeetagain Feb 20, 2023

Choose a reason for hiding this comment

twoeths Feb 20, 2023

Choose a reason for hiding this comment

maschad commented Feb 20, 2023

wemeetagain commented Feb 20, 2023

maschad commented Feb 20, 2023

maschad commented Feb 14, 2023 •

edited by wemeetagain

Loading

codecov-commenter commented Feb 15, 2023 •

edited

Loading

twoeths commented Feb 16, 2023 •

edited

Loading

wemeetagain commented Feb 20, 2023 •

edited

Loading