Skip to content

instantsend: deterministic lock using the same msg hash as islock - #4381

Merged
UdjinM6 merged 1 commit into
dashpay:developfrom
gabriel-bjg:deterministic-instantsend
Oct 5, 2021
Merged

instantsend: deterministic lock using the same msg hash as islock#4381
UdjinM6 merged 1 commit into
dashpay:developfrom
gabriel-bjg:deterministic-instantsend

Conversation

@gabriel-bjg

@gabriel-bjggabriel-bjg commented Aug 31, 2021

Copy link
Copy Markdown

When receiving an islock, propagate it as islock.
When creating/receiving an isdlock, propagate it as isdlock to peers which support it and as islock to peers which don't.
Functional tests to cover both islock and isdlock scenarios.

@gabriel-bjg
gabriel-bjgforce-pushed the deterministic-instantsend branch from 5433630 to 65dd0d2CompareAugust 31, 2021 17:20
@gabriel-bjggabriel-bjg changed the title Instant send deterministic lock uses the same msg hash as islock.Instant send deterministic lock using the same msg hash as islock.Aug 31, 2021
@gabriel-bjg
gabriel-bjgforce-pushed the deterministic-instantsend branch from 65dd0d2 to 20a0485CompareSeptember 1, 2021 06:54
@gabriel-bjg
gabriel-bjg marked this pull request as ready for review September 2, 2021 15:19
Comment threadsrc/llmq/quorums_instantsend.cpp Outdated
Comment on lines 810 to 813

@PastaPastaPastaPastaPastaPastaSep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This might be problematic.. We likely don't want to p2p ban a peer just because we don't have this block yet...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I changed it to slightly penalize a peer in order to avoid the case when some peer spams an invalid cycleHash.

Comment threadsrc/llmq/quorums_instantsend.h Outdated
Comment threadsrc/llmq/quorums_instantsend.h Outdated
Comment threadtest/functional/test_framework/messages.py Outdated
@gabriel-bjg
gabriel-bjgforce-pushed the deterministic-instantsend branch 2 times, most recently from 51124c9 to d096097CompareSeptember 9, 2021 18:11
Comment threadsrc/llmq/quorums_instantsend.cpp Outdated
@gabriel-bjg
gabriel-bjgforce-pushed the deterministic-instantsend branch 3 times, most recently from 3414356 to a049b9eCompareSeptember 16, 2021 21:25
When receiving an islock, propagate it as islock.
When creating/receiving and isdlock, propagate it as isdlock to peers which support it and as islock to peers which don't.
Functional tests to cover both islock and isdlock scenarios.
@gabriel-bjg
gabriel-bjgforce-pushed the deterministic-instantsend branch from a049b9e to 0fd8235CompareSeptember 29, 2021 07:09

@PastaPastaPastaPastaPastaPasta left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM utACK for squash merge

@PastaPastaPastaPastaPastaPasta added the P2P Some notable changes on p2p level label Oct 5, 2021
@UdjinM6UdjinM6 added this to the 18 milestone Oct 5, 2021

@UdjinM6UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

utACK

@UdjinM6UdjinM6 changed the title Instant send deterministic lock using the same msg hash as islock.instantsend: deterministic lock using the same msg hash as islock.Oct 5, 2021
@UdjinM6UdjinM6 changed the title instantsend: deterministic lock using the same msg hash as islock.instantsend: deterministic lock using the same msg hash as islockOct 5, 2021
@UdjinM6
UdjinM6 merged commit b4d001a into dashpay:developOct 5, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
…shpay#4381)
When receiving an islock, propagate it as islock.
When creating/receiving and isdlock, propagate it as isdlock to peers which support it and as islock to peers which don't.
Functional tests to cover both islock and isdlock scenarios.
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 11, 2022
…shpay#4381)
When receiving an islock, propagate it as islock.
When creating/receiving and isdlock, propagate it as isdlock to peers which support it and as islock to peers which don't.
Functional tests to cover both islock and isdlock scenarios.
PastaPastaPasta added a commit that referenced this pull request Sep 5, 2025
…er functions
a02e843 refactor: privatise `RequestObject` and `EraseObjectRequest` (Kittywhiskers Van Gogh)
f95b50a refactor: privatise `RelayInvFiltered` and `AskPeersForTransaction` (Kittywhiskers Van Gogh)
c93ade5 refactor: reduce `PeerManager` usage in InstantSend worker functions (Kittywhiskers Van Gogh)
9b2c916 refactor: drop `is_masternode` from `PeerManager` interface functions (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on #6815
* Depends on #6820
* While this pull request doesn't _entirely_ decouple `PeerManager` from `CInstantSendManager` as the worker thread isn't _reactive_ (i.e. triggered by `ProcessMessage()`), we can reduce its usage by folding all expected network activity into a `MessageProcessingResult` batch that can be consumed by `PostProcessMessage`.
To achieve this, `MessageProcessingResult` has been extended to allow
* Requesting peers for a transaction (by setting `m_request_tx`)
* Relaying inventories to interested peer based on filter match (by setting `m_inv_filter`)
This has the additional effect of removing external uses of `RelayInvFiltered()` and `AskPeersForTransaction()`, which can now be safely removed from the `PeerManager` interface as it is now brokered through `MessageProcessingResult`.
* Note that while the old `RelayInvFiltered()` allowed specifying _any_ minimum protocol version, `PostProcessMessage()` will assume that `minProtoVersion` is `ISDLOCK_PROTO_VERSION`. This is acceptable as `RelayInvFiltered()` isn't used anywhere else and deterministic InstantSend locks were introduced in v0.18 (see [dash#4381](#4381)).
If there is any foreseeable use needing to relay messages to even older versions of Dash Core, `m_inv_filter` can be modified to accommodate that.
## Breaking Changes
None expected.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK a02e843
UdjinM6:
utACK a02e843
Tree-SHA512: 6a83277781faf996cbb5187d352971c3508191dbf84cd801e062e2cde2db09cf64696bf27b9790f2b278f53adb46c042e04ac5c4dbdebdbc1faba2b262e39930
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2PSome notable changes on p2p level

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants

@gabriel-bjg@UdjinM6@PastaPastaPasta