[Mining] Use -miningaddress for in-wallet mining. #923

Zannick · 2021-04-07T20:23:58Z

Problem

-miningaddress is used for rpc progpow gpu mining, but ignored for in-wallet mining such as for RandomX and SHA256D.

Root Cause

It's just not looking at the config value when it sets up the block script.

Solution

When the wallet attempts to retrieve a mining address, prefer the configured value for -miningaddress, falling back to the previous address generation from the wallet keys.

Disallows stealth addresses as per current behavior validating -miningaddress at startup and in rpc mining.

Performs an additional safety check that the wallet actually owns the given address, falling back to a random wallet address otherwise (preserving current behavior).

Bounty PR

#867

Bounty Payment Address

sv1qqpswvjy7s9yrpcmrt3fu0kd8rutrdlq675ntyjxjzn09f965z9dutqpqgg85esvg8mhmyka5kq5vae0qnuw4428vs9d2gu4nz643jv5a72wkqqq73mnxr

Testing Results

Tested on win10 x64 mining RandomX on testnet: both with basecoin and "mining" addresses.
Tested on win10 veild, veil-qt: Using a testnet basecoin address that was not mine. Successfully produced an error and exited.

codeofalltrades

ACK bcdd0f5

CaveSpectre11

I'm having a hell of a time downloading a binary to test this, so I'll just ask a few questions:

If you specify -miningaddress with no value, it will just use a generic one and not give you an error?
If the destination is invalid, same thing?
When do you see these errors?

I think I might rather see the validity tests for -miningaddress in veild.cpp and qt/veil.cpp, so that they get feedback when they try to start the daemon or qt; and have it not continue if they don't have a valid basecoin address that is theirs. Having the differentiation between basecoin and stealth is good, so we have a hook when we do allow stealth; but if it doesn't fail on startup until they correct their use of -miningaddress, it should.

If I can get the thing to download, I'll get some of my own answers to the above questions.

CaveSpectre11

See changes needed.

CaveSpectre11 · 2021-04-19T22:28:28Z

src/wallet/wallet.cpp

+            // Disallow Stealth Addresses for now
+            CBitcoinAddress address(sAddress);
+            if (address.IsValidStealthAddress()) {
+                WalletLogPrintf("-miningaddress must be a basecoin address");
+                return GetScriptForMiningReserveKey(script);
+            }


This is unnecessary since this check is being done in veild.cpp and qt/veil.cpp

It's also being done in rpc/mining.cpp--I presume as a backstop in case the arg was changed between startup and the rpc to start mining. If it's changed, and manages to get by here, the script will be empty, and I think that will make it fail when it tries to commit a block (I tried mining to stealth on testnet at some point and iirc that's what happened). Perhaps that is user error, since we should not be mutating miningaddress in-process? The difference is that we can't return an RPC error here, and I felt it was better to fall back to existing behavior (mining to an unexpected address) rather than surprising behavior (mining fails at the last possible moment).

Cleared these checks out.

Looking back on the original addition of the stealth check, I see I did add it to the rpc area too. But that was before I moved it to init. Looks like the checks in RPC can also be removed since there isn't any way to change the address after you start the wallet up. Not sure it needs to be removed right away (it's extra code on startup that I concern myself with, vs. extra code on an rpc call)

CaveSpectre11 · 2021-04-19T22:30:25Z

src/wallet/wallet.cpp

+        if (IsValidDestination(dest)) {
+            if (IsMine(dest) != ISMINE_SPENDABLE) {
+                WalletLogPrintf("-miningaddress %s not under my control, falling back to random address", sAddress);
+                return GetScriptForMiningReserveKey(script);
+            }


These are good checks, but they should be done where miningaddress is being checked on startup.

Had to move it to init.cpp so I could be sure the wallet was loaded, but it works for both veild and veil-qt (with a delay to load the wallet, of course). Done.

Ahh yes, because of the IsMine(). Let me think about this a little bit more. I put in a good address that I didn't own, and I didn't see an error anywhere. I'm building this PR so I can do some extra debugging later.

src/wallet/wallet.cpp

Replaces the hook to get a mining address from the wallet with a retrieval of the address set as `-miningaddress`. Performs an additional safety check at startup that the wallet actually owns the given address.

Zannick

Supplying an empty miningaddress is the same as providing none, since the getArg lookups provide a default of "".
As you found, yeah, the errors are on startup. See my comments below..

src/wallet/wallet.cpp

Zannick · 2021-04-20T03:48:02Z

src/wallet/wallet.cpp

+        if (IsValidDestination(dest)) {
+            if (IsMine(dest) != ISMINE_SPENDABLE) {
+                WalletLogPrintf("-miningaddress %s not under my control, falling back to random address", sAddress);
+                return GetScriptForMiningReserveKey(script);
+            }


Had to move it to init.cpp so I could be sure the wallet was loaded, but it works for both veild and veil-qt (with a delay to load the wallet, of course). Done.

Zannick · 2021-04-20T03:48:39Z

src/wallet/wallet.cpp

+            // Disallow Stealth Addresses for now
+            CBitcoinAddress address(sAddress);
+            if (address.IsValidStealthAddress()) {
+                WalletLogPrintf("-miningaddress must be a basecoin address");
+                return GetScriptForMiningReserveKey(script);
+            }


Cleared these checks out.

CaveSpectre11 · 2021-04-20T23:35:30Z

src/init.cpp

+
+            auto pt = GetMainWallet();
+            if (pt && pt->IsMine(dest) != ISMINE_SPENDABLE) {
+                return InitError(strprintf(_("Invalid -miningaddress: '%s' not owned by wallet"), sAddress));


This has an issue; as it hangs up the wallet and doesn't exit:

2021-04-20T23:24:24Z ThreadStakeMiner: starting 2021-04-20T23:24:24Z Veil Miner started 2021-04-20T23:24:24Z Error: Invalid -miningaddress: 'bv1q7cmcjcazyxger9y9llnqpuhj33pmrn9hl0ny24' not owned by wallet 2021-04-20T23:24:24Z tor: Thread interrupt 2021-04-20T23:24:24Z torcontrol thread exit 2021-04-20T23:24:24Z opencon thread exit 2021-04-20T23:24:24Z dnsseed thread exit 2021-04-20T23:24:24Z Shutdown: In progress... 2021-04-20T23:24:24Z addcon thread exit 2021-04-20T23:24:24Z msghand thread exit 2021-04-20T23:24:24Z net thread exit 2021-04-20T23:24:24Z Imported mempool transactions from disk: 3 succeeded, 0 failed, 0 expired, 0 already there 2021-04-20T23:24:37Z ThreadStakeMiner: interrupted 2021-04-20T23:24:37Z Veil Miner started

./veil-cli stop error: Could not connect to the server 127.0.0.1:58812 Make sure the veild server is running and that you are connecting to the correct RPC port.

This appears to be an existing issue. I can reproduce it here:

veil/src/init.cpp

Lines 1972 to 1974 in 02cc068

if (!addr.IsValidStealthAddress()) {

return InitError(strprintf(_("Invalid stealth address with -autospendaddress: '%s'"), strAutoSpendAddress));

}

Let me file a separate issue...

I filed PR #935 with a fix.

codeofalltrades

ACK b2eb977
Tested on Win 10

CaveSpectre11

ACK b2eb977

But #935 must be merged first.

codeofalltrades requested review from codeofalltrades, CaveSpectre11 and WetOne April 8, 2021 14:45

codeofalltrades assigned Zannick Apr 8, 2021

codeofalltrades added Component: Miner Both PoW and PoS block creation Tag: Waiting For Code Review Waiting for code review from a core developer labels Apr 8, 2021

codeofalltrades approved these changes Apr 14, 2021

View reviewed changes

CaveSpectre11 reviewed Apr 18, 2021

View reviewed changes

CaveSpectre11 requested changes Apr 19, 2021

View reviewed changes

[Mining] Use -miningaddress for in-wallet mining.

b2eb977

Replaces the hook to get a mining address from the wallet with a retrieval of the address set as `-miningaddress`. Performs an additional safety check at startup that the wallet actually owns the given address.

Zannick force-pushed the miningaddress branch from bcdd0f5 to b2eb977 Compare April 20, 2021 03:43

Zannick commented Apr 20, 2021

View reviewed changes

CaveSpectre11 reviewed Apr 20, 2021

View reviewed changes

CaveSpectre11 added Tag: Waiting For Developer and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Apr 20, 2021

Zannick mentioned this pull request Apr 21, 2021

[Core] Some veild errors during init result in hangs #934

Closed

codeofalltrades approved these changes Apr 24, 2021

View reviewed changes

CaveSpectre11 approved these changes Apr 25, 2021

View reviewed changes

CaveSpectre11 added Code Review: Passed and removed Tag: Waiting For Developer labels Apr 25, 2021

codeofalltrades merged commit 8c80d51 into Veil-Project:master Apr 26, 2021

Zannick deleted the miningaddress branch May 18, 2021 03:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mining] Use -miningaddress for in-wallet mining. #923

[Mining] Use -miningaddress for in-wallet mining. #923

Zannick commented Apr 7, 2021 •

edited

Loading

codeofalltrades left a comment

CaveSpectre11 left a comment

CaveSpectre11 left a comment

CaveSpectre11 Apr 19, 2021

Zannick Apr 20, 2021

Zannick Apr 20, 2021

CaveSpectre11 Apr 20, 2021

CaveSpectre11 Apr 19, 2021

Zannick Apr 20, 2021

CaveSpectre11 Apr 20, 2021

Zannick left a comment

Zannick Apr 20, 2021

Zannick Apr 20, 2021

CaveSpectre11 Apr 20, 2021

Zannick Apr 21, 2021

Zannick Apr 21, 2021

codeofalltrades left a comment

CaveSpectre11 left a comment

	if (!addr.IsValidStealthAddress()) {
	return InitError(strprintf(_("Invalid stealth address with -autospendaddress: '%s'"), strAutoSpendAddress));
	}

[Mining] Use -miningaddress for in-wallet mining. #923

[Mining] Use -miningaddress for in-wallet mining. #923

Conversation

Zannick commented Apr 7, 2021 • edited Loading

Problem

Root Cause

Solution

Bounty PR

Bounty Payment Address

Testing Results

codeofalltrades left a comment

Choose a reason for hiding this comment

CaveSpectre11 left a comment

Choose a reason for hiding this comment

CaveSpectre11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zannick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeofalltrades left a comment

Choose a reason for hiding this comment

CaveSpectre11 left a comment

Choose a reason for hiding this comment

Zannick commented Apr 7, 2021 •

edited

Loading