Skip to content

remove hardcoded strDarksendPoolDummyAddress #828

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
Merged
merged 1 commit into from
May 30, 2016
Merged

remove hardcoded strDarksendPoolDummyAddress #828

merged 1 commit into from
May 30, 2016

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented May 29, 2016

  • use "" account address
  • remove collateralPubKey and SetCollateralAddress() from darksend and create dummyScriptPubkey and InitDummyScriptPubkey() in masternodeman
  • initialize dummyScriptPubkey before loading cache otherwise MNs could be removed because of the invalid tx

@schinzelh
Copy link

schinzelh commented May 29, 2016

Wow, strDarksendPoolDummyAddress - a blast from the past :)

@schinzelh
Copy link

schinzelh commented May 29, 2016 •
edited
Loading

Hmm, it seems disablewallet.py test gets stuck with these changes

https://travis-ci.org/dashpay/dash/jobs/133772479#L2708
https://travis-ci.org/dashpay/dash/jobs/133772477#L3097

Maybe a segfault with -disablewallet mode?

@UdjinM6
Copy link
Author

UdjinM6 commented May 29, 2016 •
edited
Loading

@schinzelh Not sure, travis hangs sometimes for no reason.
Anyway, fixed a bug in commit and pushed again, let's see if this can be reproduced.

EDIT: woohoo, Segmentation fault: 11 with -disablewallet :D

@UdjinM6
Copy link
Author

UdjinM6 commented May 29, 2016 •
edited
Loading

Wrapped "setup PrivateSend" step with ENABLE_WALLET and fDisableWallet (), should be ok now.
It doesn't make sense to have DS or IS without wallet anyway and we need wallet functions for MN and governance iirc. Correct me if I'm wrong pls.
@evan82

EDIT: nope, actually, having all these functions are ok without wallet for node to act as... well, a node. Need more granular control here.

@UdjinM6
remove hardcoded strDarksendPoolDummyAddress
3881103 
- use "" account address instead
- remove `collateralPubKey` and `SetCollateralAddress()` from darksend and create `dummyScriptPubkey` and `InitDummyScriptPubkey()` in masternodeman instead
- initialize `dummyScriptPubkey` before loading cache otherwise MNs could be removed because of the invalid tx
@UdjinM6
Copy link
Author

UdjinM6 commented May 29, 2016 •
edited
Loading

Ignore the message above :D
I think I came up with a way better solution - we could just generate a random address instead of using one from the wallet, see https://github.com/dashpay/dash/pull/828/files#diff-3eacbdb2557a3591cfc3764cd73bd2abR634 I also added debug.log output for this new address and corresponding private key so that user always was aware that we are not using smth pre-generated https://github.com/dashpay/dash/pull/828/files#diff-3eacbdb2557a3591cfc3764cd73bd2abR643.

@schinzelh
Copy link

schinzelh commented May 30, 2016

ACK 3881103

@schinzelh schinzelh merged commit 9745914 into dashpay:v0.12.1.x May 30, 2016
@UdjinM6 UdjinM6 deleted the removeHardcodedDSAddress branch June 20, 2016 06:45
PastaPastaPasta added a commit that referenced this pull request Aug 21, 2024
@PastaPastaPasta
Merge #6168: backport: bitcoin-core#gui18, #121, #257, #263, #281, #335…
f2940c4 
…, #362, #828, bitcoin#21912, bitcoin#21942, bitcoin#21988

c7d3161 Merge bitcoin-core/gui#362: Add keyboard shortcuts to context menus (Hennadii Stepanov)
25f87b9 Merge bitcoin-core/gui#121: Early subscribe core signals in transaction table model (Hennadii Stepanov)
ed56e28 Merge bitcoin-core/gui#335: test: Use QSignalSpy instead of QEventLoop (Hennadii Stepanov)
c52b756 Merge bitcoin-core/gui#281: set shortcuts for console's resize buttons (W. J. van der Laan)
b442a59 Merge bitcoin#21988: doc: note that brew installed qt is not supported (W. J. van der Laan)
0e2e315 Merge bitcoin#21942: docs: improve make with parallel jobs description. (MarcoFalke)
c2735a8 Merge bitcoin#21912: doc: Remove mention of priority estimation (W. J. van der Laan)
1d56d20 Merge bitcoin-core/gui#257: refactor: Use template function qOverload in signal-slot connections (Hennadii Stepanov)
b5fb559 Merge bitcoin-core/gui#18: Add peertablesortproxy module (Hennadii Stepanov)
1cdd9fb refactor: use new QAction style for governance list and masternode list (Konstantin Akimov)
4f89c98 Merge bitcoin-core/gui#263: Revamp context menus (Hennadii Stepanov)
c36bb8e fix: use && in governance urls instead & (Konstantin Akimov)
1e585b1 Merge bitcoin-core/gui#828: Rendering an amp characters in the wallet name for QMenu (Hennadii Stepanov)

Pull request description:

  ## Issue being fixed or feature implemented
  Just regular backports from bitcoin v22, mostly Qt related

  ## What was done?
  See commits for a list of backports.

  This PR also fixes a rendering an url on Governance tab if it has any '&' inside. see screenshots.

  Original url: `https://example.com/?test=nothing&to=see&&lol` - yes, double '&&' just for test even if url is silly.
  Failed behaviour:
  ![image](https://github.com/user-attachments/assets/ac45c192-7d0e-4cd2-97f8-060af8f3911b)
  Correctly rendered:
  ![image](https://github.com/user-attachments/assets/5e345197-776a-4bb8-9476-cab4aba3429e)

  ## How Has This Been Tested?
  Run unit/functional tests

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] 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

ACKs for top commit:
  PastaPastaPasta:
    utACK c7d3161
  UdjinM6:
    utACK c7d3161

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@UdjinM6 @schinzelh