Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: store ready queues on the mixing masternode #6452

Merged
merged 1 commit into from
Dec 6, 2024
Merged

fix: store ready queues on the mixing masternode #6452

merged 1 commit into from
Dec 6, 2024

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Dec 5, 2024 •
edited
Loading

Issue being fixed or feature implemented

We normally do not re-relay/store "ready" dsq-s because these are ment to be relayed between mixing clients and the mixing masternode only. I works ok when you simply send dsq messages because no extra steps are required. However since 70235 we send dsq inv-s first and we send actual dsq-s only if they are requested via getdata. The problem here is that ProcessGetData() queries vecCoinJoinQueue via GetQueueFromHash() to get the data to send back but there is none because we never saved it.

What was done?

How Has This Been Tested?

To test this patch you need a MN but you can test 2 cases without this patch to indirectly test the idea:

  1. try mixing on develop: almost no mixing txes, maybe 10 or so in an hour if you are lucky to mix on an old MN that often
  2. ignore old MNs (MIN_PEER_PROTO_VERSION = 70235): 0 mixing txes, no matter how long you wait
  3. pretend being an old client to receive no-inv dsq only (PROTOCOL_VERSION = 70233): no issues, mixing on these nodes is as fast as usual, several txes per block (need a couple of nodes like that on the network so that a mixing session could be completed, I'm running one node for now so that anyone could join and test it)

I'm running a testnet MN with this fix and applied "ignore old mn" patch on my local machine. Local wallet got mixing tx when it finally hit the patched mn. Also confirmed this via MNdebug.log (Create/Relay/CommitFinalTransaction in logs).

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@UdjinM6 UdjinM6 added this to the 22 milestone Dec 5, 2024
@knst
Copy link
Collaborator

knst commented Dec 5, 2024

will CJ work without this patch on v22?

@UdjinM6
Copy link
Author

UdjinM6 commented Dec 5, 2024

will CJ work without this patch on v22?

No. I believe this bug is causing CJ issues on testnet right now - most of MNs there are on 70235 so very few mixing sessions succeed. Pls check the updated How Has This Been Tested? in PR description for more info.

PastaPastaPasta
PastaPastaPasta approved these changes Dec 5, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK 24dcce9

knst
knst approved these changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

LGTM 24dcce9

@PastaPastaPasta PastaPastaPasta merged commit 1f58adc into dashpay:develop Dec 6, 2024
24 checks passed
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Dec 6, 2024
@PastaPastaPasta
Merge dashpay#6452: fix: store ready queues on the mixing masternode
0ad0e60 
24dcce9 fix: store ready queues on the mixing masternode (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  We normally do not re-relay/store "ready" `dsq`-s because these are ment to be relayed between mixing clients and the mixing masternode only. I works ok when you simply send `dsq` messages because no extra steps are required. However since 70235 we send `dsq` _`inv`-s_ first and we send actual `dsq`-s only if they are requested via `getdata`. The problem here is that `ProcessGetData()` queries `vecCoinJoinQueue` via `GetQueueFromHash()` to get the data to send back but there is none because we never saved it.

  ## What was done?

  ## How Has This Been Tested?
  To test this patch you need a MN but you can test 2 cases _without this patch_ to indirectly test the idea:
  1. try mixing on develop: almost no mixing txes, maybe 10 or so in an hour if you are lucky to mix on an old MN that often
  2. ignore old MNs (`MIN_PEER_PROTO_VERSION = 70235`): 0 mixing txes, no matter how long you wait
  3. pretend being an old client to receive no-inv `dsq` only (`PROTOCOL_VERSION = 70233`): no issues, mixing on these nodes is as fast as usual, several txes per block (need a couple of nodes like that on the network so that a mixing session could be completed, I'm running one node for now so that anyone could join and test it)

  I'm running a testnet MN with this fix and applied "ignore old mn" patch on my local machine. Local wallet got mixing tx when it finally hit the patched mn. Also confirmed this via MN`debug.log` (`Create`/`Relay`/`CommitFinalTransaction` in logs).

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 24dcce9

Tree-SHA512: 69cee5401d26eec3f66166a754b8020e7f550dac4a0fdea8ec48ea1082f1286e647ac0a26a189c4d39e1a9da4e7ac36f71913684b13ea0fb4b3cfe831174970e
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Dec 6, 2024
@PastaPastaPasta
Merge dashpay#6452: fix: store ready queues on the mixing masternode
758cd64 
24dcce9 fix: store ready queues on the mixing masternode (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  We normally do not re-relay/store "ready" `dsq`-s because these are ment to be relayed between mixing clients and the mixing masternode only. I works ok when you simply send `dsq` messages because no extra steps are required. However since 70235 we send `dsq` _`inv`-s_ first and we send actual `dsq`-s only if they are requested via `getdata`. The problem here is that `ProcessGetData()` queries `vecCoinJoinQueue` via `GetQueueFromHash()` to get the data to send back but there is none because we never saved it.

  ## What was done?

  ## How Has This Been Tested?
  To test this patch you need a MN but you can test 2 cases _without this patch_ to indirectly test the idea:
  1. try mixing on develop: almost no mixing txes, maybe 10 or so in an hour if you are lucky to mix on an old MN that often
  2. ignore old MNs (`MIN_PEER_PROTO_VERSION = 70235`): 0 mixing txes, no matter how long you wait
  3. pretend being an old client to receive no-inv `dsq` only (`PROTOCOL_VERSION = 70233`): no issues, mixing on these nodes is as fast as usual, several txes per block (need a couple of nodes like that on the network so that a mixing session could be completed, I'm running one node for now so that anyone could join and test it)

  I'm running a testnet MN with this fix and applied "ignore old mn" patch on my local machine. Local wallet got mixing tx when it finally hit the patched mn. Also confirmed this via MN`debug.log` (`Create`/`Relay`/`CommitFinalTransaction` in logs).

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 24dcce9

Tree-SHA512: 69cee5401d26eec3f66166a754b8020e7f550dac4a0fdea8ec48ea1082f1286e647ac0a26a189c4d39e1a9da4e7ac36f71913684b13ea0fb4b3cfe831174970e
PastaPastaPasta added a commit that referenced this pull request Dec 6, 2024
@PastaPastaPasta
Merge #6454: [v22.0.x] backport: 22.0.0 rc.4 backports
9d3f22a 
fa29ed5 Merge #6456: fix(qt): allow refreshing wallet data without crashing (pasta)
758cd64 Merge #6452: fix: store ready queues on the mixing masternode (pasta)
395447b Merge #6451: depends: update 'src/dashbls' to dashpay/bls-signatures@7e747e8a as 62fa665 (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  batch of backports to go into rc.4

  ## What was done?
  Bls library updated to compile on freebsd, fix for mixing

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK fa29ed5

Tree-SHA512: 6a050bca13ca2e5324a6a8a7966d2d6aa3c0c97ee3c884aa35102f949dfef62e976d053cd05a549908c30e8bb6a81d996a82181852841809d8959ca78c96e823
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Dec 14, 2024
@PastaPastaPasta
Merge dashpay#6484: chore: Merge master 22.0.0 back into develop
779e429 
1c7bfcb chore: set release true (pasta)
c90339e Merge dashpay#6459: docs: add release notes for v22.0.0 (pasta)
a6f1fc5 Merge dashpay#6475: chore: bumped chain assumed sizes based on latest usage (pasta)
d7cd9f1 Merge dashpay#6464: chore: update man pages for v22 (pasta)
212f91c Merge dashpay#6461: docs: update supported versions in SECURITY.md (pasta)
9a8b685 Merge dashpay#6460: chore: Translations 2024-12 (pasta)
2f71f4d Merge dashpay#6458: chore: bump MIN_MASTERNODE_PROTO_VERSION to latest proto (pasta)
fa29ed5 Merge dashpay#6456: fix(qt): allow refreshing wallet data without crashing (pasta)
758cd64 Merge dashpay#6452: fix: store ready queues on the mixing masternode (pasta)
395447b Merge dashpay#6451: depends: update 'src/dashbls' to dashpay/bls-signatures@7e747e8a as 62fa665 (pasta)
c7b0d80 Merge dashpay#6441: fix: hold wallet shared pointer in CJ Manager/Sessions to prevent concurrent unload (pasta)
c074e09 Merge dashpay#6444: fix: add platform transfer to "most common" filter (pasta)
cb04114 Merge dashpay#6442: fix: coin selection with `include_unsafe` option should respect `nCoinType` (pasta)
db5b53a Merge dashpay#6434: fix: early EHF and buried EHF are indistinguish (pasta)
8b88ff7 Merge dashpay#6414: chore: bump seeds for v22 (pasta)
02ad523 Merge dashpay#6411: chore: update nMinimumChainWork, defaultAssumeValid, checkpointData, chainTxData for mainnet and testnet (pasta)
3bbcd3d Merge dashpay#6393: docs: mention building for some HOSTs only in `release-process.md` (pasta)
18f636f Merge dashpay#6426: fix: respect SENDDSQUEUE message, move DSQ relay into net processing / peerman (pasta)
9fed456 Merge dashpay#6407: fix: dataraces (pasta)
86105da Merge dashpay#6408: refactor: removed pre-MN_RR logic of validation of CL (pasta)
a1f7e96 Merge dashpay#6406: ci: use `actions/cache` to manage depends cache (pasta)
90a3807 Merge dashpay#6402: ci: cache built (pasta)
66f6787 Merge dashpay#6401: ci: deduplicate depends building (pasta)
7ca5663 Merge dashpay#6397: ci: add powerpc64 to GH Guix job matrix (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  ## What was done?
  Suppressed changes from 1c7bfcb and resolved merge conflicts.

  ```
  Auto-merging .github/workflows/build.yml
  Auto-merging configure.ac
  Auto-merging src/chainparams.cpp
  Auto-merging src/coinjoin/client.cpp
  CONFLICT (content): Merge conflict in src/coinjoin/client.cpp
  Auto-merging src/coinjoin/client.h
  CONFLICT (content): Merge conflict in src/coinjoin/client.h
  Auto-merging src/coinjoin/util.cpp
  CONFLICT (content): Merge conflict in src/coinjoin/util.cpp
  Auto-merging src/coinjoin/util.h
  CONFLICT (content): Merge conflict in src/coinjoin/util.h
  Auto-merging src/evo/specialtxman.cpp
  Auto-merging src/init.cpp
  Auto-merging src/net_processing.cpp
  CONFLICT (content): Merge conflict in src/net_processing.cpp
  Auto-merging src/net_processing.h
  Auto-merging src/qt/transactiontablemodel.cpp
  Auto-merging src/wallet/wallet.cpp
  Auto-merging src/wallet/wallet.h
  CONFLICT (content): Merge conflict in src/wallet/wallet.h
  Auto-merging test/functional/feature_llmq_chainlocks.py
  CONFLICT (content): Merge conflict in test/functional/feature_llmq_chainlocks.py
  ```

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK d108579; no diff to develop

Tree-SHA512: 3f063011224880fee35edb04ce265dff33a52273c3d45ef1dbcebcecb22c25d8ad7c91b83514f36142716a6fbd0ddd3a8a3f2a9b59ce78ce975bbce69a2a13b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@knst knst knst approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
22
Development

Successfully merging this pull request may close these issues.
3 participants
@UdjinM6 @knst @PastaPastaPasta