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

Core14 changes #846

Merged
merged 6 commits into from
Jul 17, 2020
Merged

Core14 changes #846

merged 6 commits into from
Jul 17, 2020

Conversation

riordant
Copy link
Contributor

@riordant riordant commented May 31, 2020 •
edited
Loading

  • make CHDMintWallet a member of CWallet (to better handle multiple wallet architecture in core 14)

  • Mac test optimisations:

    • make walletDB object a part of CHDMintWallet, pass as reference where possible
    • minimuse file I/O

  • some HDMint code cleanup

@lgtm-com
Copy link

lgtm-com bot commented May 31, 2020

This pull request introduces 1 alert when merging ea35dc7 into 1a5763f - view on LGTM.com

new alerts:

  • 1 for Resource not released in destructor

@riordant riordant force-pushed the core14-zwallet-fix branch from ea35dc7 to 73edfd7 Compare June 1, 2020 06:45
Base automatically changed from core14 to master June 1, 2020 08:46
@psolstice psolstice force-pushed the core14-zwallet-fix branch from 06352af to 320fbde Compare June 1, 2020 12:11
@riordant riordant mentioned this pull request Jun 1, 2020
Merged
@lgtm-com
Copy link

lgtm-com bot commented Jun 1, 2020

This pull request introduces 1 alert when merging 5e75d24 into e44462b - view on LGTM.com

new alerts:

  • 1 for Resource not released in destructor

@lgtm-com
Copy link

lgtm-com bot commented Jun 2, 2020

This pull request introduces 1 alert when merging b9128f3 into c156819 - view on LGTM.com

new alerts:

  • 1 for Resource not released in destructor

@riordant riordant force-pushed the core14-zwallet-fix branch from 0b00090 to 0045c3b Compare June 2, 2020 13:31
ultimaweapon
ultimaweapon reviewed Jun 3, 2020
View reviewed changes
src/wallet/wallet.h Outdated
@@ -751,6 +750,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
MasterKeyMap mapMasterKeys;
unsigned int nMasterKeyMaxID;

CHDMintWallet* zwallet;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will cause memory leak on some places due to it re-assign this without freeing the previos value. Better to to use std::unique_ptr instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value will not be reassigned within the same instance ever, but a unique_ptr is better anyway. Updated, please take a look

@riordant riordant force-pushed the core14-zwallet-fix branch from 1757a53 to 8b4221e Compare June 3, 2020 05:10
@reubenyap reubenyap added this to the v0.14.1.0 milestone Jun 3, 2020
levonpetrosyan93
levonpetrosyan93 previously approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@levonpetrosyan93 levonpetrosyan93 left a comment

Choose a reason for hiding this comment

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

LGTM

@lgtm-com
Copy link

lgtm-com bot commented Jun 3, 2020

This pull request fixes 1 alert when merging bf6d3b6 into 3de577b - view on LGTM.com

fixed alerts:

  • 1 for Unused import

@riordant riordant force-pushed the core14-zwallet-fix branch from bf6d3b6 to a7a6d1c Compare June 3, 2020 10:28
@riordant
Copy link
Contributor Author

riordant commented Jun 3, 2020

@levonpetrosyan93 @ultimaweapon rebased, some issues so had to make a new commit. please re-review.

psolstice
psolstice requested changes Jun 8, 2020
View reviewed changes
src/wallet/wallet.cpp Outdated
@@ -102,7 +102,7 @@ struct CompareByAmount

static void EnsureMintWalletAvailable()
{
if (!zwalletMain) {
if (!pwalletMain->zwallet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to ensure pwalletMain is not null here

src/wallet/wallet.cpp Outdated
@@ -175,7 +175,7 @@ CPubKey CWallet::GetKeyFromKeypath(uint32_t nChange, uint32_t nChild) {
return pubkey;
}

CPubKey CWallet::GenerateNewKey(uint32_t nChange)
CPubKey CWallet::GenerateNewKey(uint32_t nChange, bool nWriteChain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you start boolean variables with n? In bitcoin it's f to denote variable as a "flag"

src/wallet/rpcwallet.cpp Outdated
@@ -84,7 +84,7 @@ bool EnsureWalletIsAvailable(CWallet * const pwallet, bool avoidException)

void EnsureSigmaWalletIsAvailable()
{
if (!zwalletMain) {
if (!pwalletMain->zwallet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to check pwalletMain for null

src/hdmint/wallet.cpp Outdated
@@ -521,7 +520,7 @@ CKeyID CHDMintWallet::GetMintSeedID(int32_t nCount){
* @param seedId (optional) seedId of the key to use for mint generation.
* @return sucess
*/
bool CHDMintWallet::CreateMintSeed(uint512& mintSeed, const int32_t& nCount, CKeyID& seedId)
bool CHDMintWallet::CreateMintSeed(CWalletDB& walletdb, uint512& mintSeed, const int32_t& nCount, CKeyID& seedId, bool nWriteChain)
Copy link
Contributor

Choose a reason for hiding this comment

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

nWriteChain -> fWriteChain

src/wallet/sigmaspendbuilder.cpp Outdated
@@ -168,13 +168,14 @@ CAmount SigmaSpendBuilder::GetChanges(std::vector<CTxOut>& outputs, CAmount amou
auto params = sigma::Params::get_default();

CHDMint hdMint;
CWalletDB walletdb(pwalletMain->strWalletFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect in multiple wallets scenario. You need to pass CWalletDB object reference to GetChanges

Copy link
Contributor

Choose a reason for hiding this comment

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

Or you can obtain wallet file name from CWallet object and use it to create CWalletDB

psolstice
psolstice requested changes Jun 12, 2020
View reviewed changes
src/wallet/rpcwallet.cpp Outdated
@@ -84,7 +84,7 @@ bool EnsureWalletIsAvailable(CWallet * const pwallet, bool avoidException)

void EnsureSigmaWalletIsAvailable()
{
if (!pwalletMain->zwallet) {
if (!pwalletMain && !pwalletMain->zwallet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

||, not &&

src/wallet/wallet.cpp Outdated
@@ -102,7 +102,7 @@ struct CompareByAmount

static void EnsureMintWalletAvailable()
{
if (!pwalletMain->zwallet) {
if (!pwalletMain && !pwalletMain->zwallet) {
Copy link
Contributor

Choose a reason for hiding this comment

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

&& -> ||

@riordant riordant force-pushed the core14-zwallet-fix branch from e0ff003 to 149e41b Compare June 15, 2020 06:47
@reubenyap reubenyap merged commit 2d18620 into master Jul 17, 2020
@reubenyap reubenyap deleted the core14-zwallet-fix branch June 9, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ultimaweapon ultimaweapon ultimaweapon left review comments

@levonpetrosyan93 levonpetrosyan93 levonpetrosyan93 approved these changes

@psolstice psolstice psolstice approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@riordant @ultimaweapon @psolstice @levonpetrosyan93 @reubenyap