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.

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

Add name_autoregister RPC #436

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
335 changes: 312 additions & 23 deletions src/wallet/rpcnames.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@
#include <util/vector.h>
#include <validation.h>
#include <wallet/coincontrol.h>
#include <wallet/rpcnames.h>
#include <wallet/rpcwallet.h>
#include <wallet/scriptpubkeyman.h>
#include <wallet/spend.h>
#include <wallet/wallet.h>

#include <univalue.h>
Expand Down Expand Up @@ -355,10 +357,309 @@ saltMatchesHash(const valtype& name, const valtype& rand, const valtype& expecte
return (Hash160(toHash) == uint160(expectedHash));
}

bool existsName(const valtype& name, const ChainstateManager& chainman)
{
LOCK(cs_main);

Choose a reason for hiding this comment

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

I know you just moved this code, but I'm wondering - since this now uses the ChainstateManager, do we actually need cs_main? It seems that ActiveChainstate by itself acquires the lock (but this is an implementation detail that might be changed upstream with more refactoring / de-globalising the chainstate). So I think you can remove the lock here.

const auto& coinsTip = chainman.ActiveChainstate().CoinsTip();
CNameData oldData;
return (coinsTip.GetName(name, oldData) && !oldData.isExpired(chainman.ActiveHeight()));
}

bool existsName(const std::string& name, const ChainstateManager& chainman)
{
return existsName(valtype(name.begin(), name.end()), chainman);
}

} // anonymous namespace

/* ************************************************************************** */

RPCHelpMan
name_autoregister()
{
NameOptionsHelp optHelp;
optHelp
.withNameEncoding()
.withWriteOptions()
.withArg("allowExisting", RPCArg::Type::BOOL, "false",
"If set, then the name_new is sent even if the name exists already")
.withArg("delegate", RPCArg::Type::BOOL, "false",
"If set, register a dd/ or idd/ name and delegate the name there. Name must be in d/ or id/ namespace.");

return RPCHelpMan("name_autoregister",
"\nAutomatically registers the given name; performs the first half and queues the second."
+ HELP_REQUIRING_PASSPHRASE,
{
{"name", RPCArg::Type::STR, RPCArg::Optional::NO, "The name to register"},
{"value", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Value for the name"},
optHelp.buildRpcArg(),
},
RPCResult {RPCResult::Type::ARR_FIXED, "", "",

Choose a reason for hiding this comment

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

Is there a particular reason why we want to return the result as array? Since this is a new RPC method, there is not really any API-compatibility to keep. I think returning a JSON object instead of that legacy array makes a lot more sense.

{
{RPCResult::Type::STR_HEX, "txid", "the txid, used in name_firstupdate"},
{RPCResult::Type::STR_HEX, "rand", "random value, as in name_firstupdate"},
},
},
RPCExamples {
HelpExampleCli("name_autoregister", "\"myname\"")
+ HelpExampleRpc("name_autoregister", "\"myname\"")
},
[&] (const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
if (!wallet)
return NullUniValue;
CWallet* const pwallet = wallet.get();

RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VSTR, UniValue::VOBJ});
const auto& chainman = EnsureChainman(EnsureAnyNodeContext(request));

// build a name_new
// broadcast it
// build a name_fu with nSequence
// queue it
// return the txid and rand from name_new, as an array
//
// if delegate=true:
// build nn1 = d/something
// build nn2 = dd/something (if already exist, something else)
// broadcast nn1,nn2
// build nfu1 = "d/something points to dd/xxx"
// build nfu2 = "d/xxx is the value"
// queue nfu1,nfu2
// return txid from nn1

Choose a reason for hiding this comment

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

Is that delegation stuff something we really want to do in Namecoin Core itself? So far, we've managed to keep Namecoin Core agnostic to any kind of values or names, and handle all namespace and name-value-JSON stuff on a higher level. I'd prefer to keep it that way.

Copy link
Author

Choose a reason for hiding this comment

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

That's a fair point. I only added it because there was funding to do it. I agree with you there's something slightly ungainly about Namecoin Core going this far, and in particular about generating the JSON.

I'm not wholesale opposed to having it in, because it's better to have one good implementation in Namecoin Core than twelve thousand shoddy ones that never get maintained in various downstream consumers, but I think such functionality (that has to be aware of namespace rules etc) should at the very least be quite cleanly split off, like the proposal #365. (For delegation, having consumers issue two autoregisters and moving that further downstream honestly seems fine)

Maybe it could be a separate RPC, or behind a #ifdef directive?

Choose a reason for hiding this comment

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

As you say, for delegation it is completely irrelevant from the consumer's side, and that's what it is doing for now, so I would just leave that to the caller. (And if you use it from a "name registration" Qt UI, then the JSON generation should be in the Qt code, not the RPC one.)

Are you planning to add more complex JSON generation in the future? In that case, I believe perhaps a separate library (in Python or Node or whatever typical consumers will use) that can be hooked onto the RPC interface makes the most sense.

Copy link
Member

Choose a reason for hiding this comment

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

Is that delegation stuff something we really want to do in Namecoin Core itself? So far, we've managed to keep Namecoin Core agnostic to any kind of values or names, and handle all namespace and name-value-JSON stuff on a higher level. I'd prefer to keep it that way.

@domob1812 I strongly disagree with this. From a UX perspective, it is important for the Namecoin-Qt GUI to handle this, without expecting the user to go into a separate GUI. (E.g. Tor Project policy is that user perception of the Tor/Firefox segregation should be minimized, because that segregation confuses users.) In theory there could be a separate daemon that handle this, and accepts commands from Namecoin-Qt in a way that the user doesn't perceive, but the main advantage of such a thing is security/isolation, and I am not convinced that this is a case where that's needed. (In fact, this functionality is so simple compared to the needed IPC logic that it may actually expose more attack surface to keep this in a separate process.)

Putting this functionality in the Qt code rather than RPC has the primary effect of making integration tests harder, so I am not convinced that that is a good idea. (AIUI, Bitcoin Core policy is that functionality must be added to RPC before being added to Qt, so that integration testing is easier.)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, but it's still gross, even if you have to do it for UX needs.

I don't even see why the GUI has to handle this. Can't users just make two domains by themselves? Are they that stupid? But if it does, then I don't see why it can't be (1) a separate RPC or (2) a .so library you load.

Because there is a third reason, which is to segregate it to keep the codebase clean.

I think I'd prefer to have two RPCs for manipulating names (parse name, serialize name), and two RPCs for manipulating values (decode value, encode value). Then you could factor out "find name by appending random digits" into a RPC call, and then just put the parts together in the GUI.


UniValue options(UniValue::VOBJ);
if (request.params.size() >= 3)
options = request.params[2].get_obj();
RPCTypeCheckObj(options,
{
{"allowExisting", UniValueType(UniValue::VBOOL)},
{"delegate", UniValueType(UniValue::VBOOL)},
},
true, false);

const valtype name = DecodeNameFromRPCOrThrow(request.params[0], options);
if (name.size() > MAX_NAME_LENGTH)
throw JSONRPCError(RPC_INVALID_PARAMETER, "the name is too long");

const bool isDefaultVal = (request.params.size() < 2 || request.params[1].isNull());
const valtype value = isDefaultVal ?
valtype():
DecodeValueFromRPCOrThrow(request.params[1], options);

if (value.size() > MAX_VALUE_LENGTH_UI)
throw JSONRPCError(RPC_INVALID_PARAMETER, "the value is too long");

if (!options["allowExisting"].isTrue())
{
LOCK(cs_main);

Choose a reason for hiding this comment

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

No need for this lock I think.

if (existsName(name, chainman))
throw JSONRPCError(RPC_TRANSACTION_ERROR, "this name exists already");
}

// TODO: farm out to somewhere else for namespace parsing

Choose a reason for hiding this comment

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

As per the discussion above, I really think we should not merge a change like this now with just a TODO to remove the logic again in the future. Instead, I think we should first merge this completely without delegation support, and then add it in in a proper way later.


bool isDelegated = options["delegate"].isTrue();
std::string delegatedName;
std::string delegatedValue;

if (isDelegated)
{
bool isDomain;
bool isIdentity;
std::string nameStr(name.begin(), name.end());
isDomain = nameStr.rfind("d/", 0) == 0;
isIdentity = nameStr.rfind("id/", 0) == 0;

if (!isDomain && !isIdentity)
throw JSONRPCError(RPC_INVALID_PARAMETER, "delegation requested, but name neither d/ nor id/");

assert(!(isDomain && isIdentity));

size_t slashIdx = nameStr.find_first_of('/');
assert(slashIdx != std::string::npos);

std::string mainLabel = nameStr.substr(slashIdx, std::string::npos);

std::string prefix = isDomain ? "dd" : "idd";

std::string suffix("");

// Attempt to generate name like dd/name, dd/name5, dd/name73
do {
delegatedName = prefix + mainLabel + suffix;
valtype rand(1);
GetRandBytes(&rand[0], rand.size());
suffix += std::string(1, '0' + (rand[0] % 10));
} while (existsName(delegatedName, chainman) && delegatedName.size() <= MAX_NAME_LENGTH);

// Fallback. This could happen if the base name is 254 characters, for instance
while (existsName(delegatedName, chainman) || delegatedName.size() > MAX_NAME_LENGTH) {
// Attempt to generate name like dd/f7f5fdbd
valtype rand(4);
GetRandBytes(&rand[0], rand.size());
delegatedName = strprintf("%s/%hh02x%hh02x%hh02x%hh02x", prefix, rand[0], rand[1], rand[2], rand[3]);
// TODO: Escape properly for JSON.
}

delegatedValue = strprintf("{\"import\":\"%s\"}", delegatedName);
}

UniValue res(UniValue::VARR);

/* Make sure the results are valid at least up to the most recent block
the user could have gotten from another RPC command prior to now. */
pwallet->BlockUntilSyncedToCurrentChain();

auto issue_nn = [&] (const valtype name, bool push) {

Choose a reason for hiding this comment

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

Overall, this new method is far too large for a single method to ensure code readability. I think instead of using the closures here, this whole thing should be refactored into a helper class instead. The closures can be turned into member functions, and all variables referenced in the closure would be member variables.

Then the main autoregister method would do basic parameter parsing, instantiate the helper class, and then just call the helper methods according to the high-level flow. This would ensure that each individual method is self-contained and relatively short.

LOCK(pwallet->cs_wallet);
EnsureWalletIsUnlocked(*pwallet);

DestinationAddressHelper destHelper(*pwallet);
destHelper.setOptions(options);

const CScript output = destHelper.getScript();

valtype rand(20);
if (!getNameSalt(pwallet, name, output, rand))
GetRandBytes(&rand[0], rand.size());

const CScript newScript
= CNameScript::buildNameNew(output, name, rand);

const UniValue txidVal
= SendNameOutput(request, *pwallet, newScript, nullptr, options);
// TODO: Refactor away SendNameOutput and move the locking here.
destHelper.finalise();

const std::string randStr = HexStr(rand);
const std::string txid = txidVal.get_str();
LogPrintf("name_new: name=%s, rand=%s, tx=%s\n",
EncodeNameForMessage(name), randStr.c_str(), txid.c_str());

if (push) {
res.push_back(txid);
res.push_back(randStr);
}

return std::make_pair(uint256S(txid), rand);
};

auto locktxid = [&] (const uint256 txid) {
LOCK(pwallet->cs_wallet);

const CWalletTx* wtx = pwallet->GetWalletTx(txid);
CTxIn txIn;

for (auto& in : wtx->tx->vin) {
pwallet->LockCoin(in.prevout);
}
for (auto& in : wtx->tx->vin) {
CWalletTx &coin = pwallet->mapWallet.at(in.prevout.hash);
coin.MarkDirty();
pwallet->NotifyTransactionChanged(coin.GetHash(), CT_UPDATED);
}

pwallet->CommitTransaction(wtx->tx, {}, {});
};

auto queue_nfu = [&] (const valtype name, const valtype value, const auto info) {
LOCK(pwallet->cs_wallet);
const int TWELVE_PLUS_ONE = 13;

uint256 txid = info.first;
valtype rand = info.second;

const CWalletTx* wtx = pwallet->GetWalletTx(txid);
CTxIn txIn;

for (unsigned int i = 0; i < wtx->tx->vout.size(); i++)
if (CNameScript::isNameScript(wtx->tx->vout[i].scriptPubKey)) {
txIn = CTxIn(COutPoint(txid, i), wtx->tx->vout[i].scriptPubKey, /* nSequence */ TWELVE_PLUS_ONE);
// nSequence = 13 => only broadcast name_firstupdate when name_new is mature (12 blocks)
// Note: nSequence is basically ornamental here, see comment below
}

EnsureWalletIsUnlocked(*pwallet);

DestinationAddressHelper destHelper(*pwallet);
destHelper.setOptions(options);

// if delegated, use delegationValue for value, and use value in dd/ name
const CScript nameScript
= CNameScript::buildNameFirstupdate(destHelper.getScript(), name, value, rand);

CAmount nFeeRequired = 0;
int nChangePosRet = -1;
bilingual_str error;
CTransactionRef tx;
FeeCalculation fee_calc_out;

CCoinControl coin_control;

std::vector<CRecipient> recipients;
recipients.push_back({nameScript, NAME_LOCKED_AMOUNT, false});

const bool created_ok = CreateTransaction(*pwallet, recipients, &txIn, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, /* sign */ false);
if (!created_ok)
throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original);
// Not sure if this can ever happen.

// No need to sign; sigature will be discarded.

// Kludge: Since CreateTransaction discards nSequence of txIn, manually add it back in again.

CMutableTransaction mtx(*tx);

for (unsigned int i = 0; i < mtx.vin.size(); i++)
if (mtx.vin[i].prevout == txIn.prevout)
mtx.vin[i].nSequence = TWELVE_PLUS_ONE;

// Sign it for real
bool complete = pwallet->SignTransaction(mtx);
if (!complete)
throw JSONRPCError(RPC_WALLET_ERROR, "Error signing transaction");
// This should never happen.

// TODO: Mark all inputs unspendable.

CTransactionRef txr = MakeTransactionRef(mtx);

pwallet->AddToWallet(txr, /* confirm */ {}, /* update_wtx */ nullptr, /* fFlushOnClose */ true);
// If the transaction is not added to the wallet, the inputs will continue to
// be considered spendable, causing us to double-spend the most preferable input if delegating.

const bool queued_ok = pwallet->WriteQueuedTransaction(mtx.GetHash(), mtx);
if (!queued_ok)
throw JSONRPCError(RPC_WALLET_ERROR, "Error queueing transaction");

destHelper.finalise();

return std::make_pair(txr->GetHash(), nullptr);
};

auto info = issue_nn(name, true);
locktxid(info.first);
if (isDelegated) {
auto info2 = issue_nn(valtype(delegatedName.begin(), delegatedName.end()), false);
locktxid(info2.first);
auto ia = queue_nfu(name, valtype(delegatedValue.begin(), delegatedValue.end()), info);
locktxid(ia.first);
auto ib = queue_nfu(valtype(delegatedName.begin(), delegatedName.end()), value, info2);
locktxid(ib.first);
}
else {
queue_nfu(name, value, info);
}

return res;
}
);
}

/* ************************************************************************** */

RPCHelpMan
name_new ()
{
Expand Down Expand Up @@ -409,15 +710,9 @@ name_new ()
if (name.size () > MAX_NAME_LENGTH)
throw JSONRPCError (RPC_INVALID_PARAMETER, "the name is too long");

if (!options["allowExisting"].isTrue ())
{
LOCK (cs_main);
CNameData oldData;
const auto& coinsTip = chainman.ActiveChainstate ().CoinsTip ();
if (coinsTip.GetName (name, oldData)
&& !oldData.isExpired (chainman.ActiveHeight ()))
throw JSONRPCError (RPC_TRANSACTION_ERROR, "this name exists already");
}
if (!options["allowExisting"].isTrue () &&
existsName (name, chainman))
throw JSONRPCError (RPC_TRANSACTION_ERROR, "this name exists already");

/* Make sure the results are valid at least up to the most recent block
the user could have gotten from another RPC command prior to now. */
Expand Down Expand Up @@ -585,16 +880,10 @@ name_firstupdate ()
"this name is already being registered");
}

if (request.params.size () < 6 || !request.params[5].get_bool ())
{
LOCK (cs_main);
CNameData oldData;
const auto& coinsTip = chainman.ActiveChainstate ().CoinsTip ();
if (coinsTip.GetName (name, oldData)
&& !oldData.isExpired (chainman.ActiveHeight ()))
throw JSONRPCError (RPC_TRANSACTION_ERROR,
"this name is already active");
}
if ((request.params.size () < 6 || !request.params[5].get_bool ()) &&
existsName (name, chainman))
throw JSONRPCError (RPC_TRANSACTION_ERROR,
"this name is already active");

uint256 prevTxid = uint256::ZERO; // if it can't find a txid, force an error
if (fixedTxid)
Expand Down Expand Up @@ -798,12 +1087,12 @@ name_update ()
{
LOCK (cs_main);

CNameData oldData;
const auto& coinsTip = chainman.ActiveChainstate ().CoinsTip ();
if (!coinsTip.GetName (name, oldData)
|| oldData.isExpired (chainman.ActiveHeight ()))
if (!existsName (name, chainman))
throw JSONRPCError (RPC_TRANSACTION_ERROR,
"this name can not be updated");
CNameData oldData;
const auto& coinsTip = chainman.ActiveChainstate ().CoinsTip ();
coinsTip.GetName (name, oldData);

Choose a reason for hiding this comment

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

I think we should check the return value here. It is supposed to be always true (since we now check for existsName before), but this should be asserted. Note that you need to store the return value in a variable and then assert on it, rather than assert directly, to make sure there are no side-effects in the assert statement itself.

if (isDefaultVal)
value = oldData.getValue();
outp = oldData.getUpdateOutpoint ();
Expand Down
2 changes: 2 additions & 0 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5054,6 +5054,7 @@ RPCHelpMan importdescriptors();
RPCHelpMan listdescriptors();

extern RPCHelpMan name_list(); // in rpcnames.cpp
extern RPCHelpMan name_autoregister();
extern RPCHelpMan name_new();
extern RPCHelpMan name_firstupdate();
extern RPCHelpMan name_update();
Expand Down Expand Up @@ -5139,6 +5140,7 @@ static const CRPCCommand commands[] =

// Name-related wallet calls.
{ "names", &name_list, },
{ "names", &name_autoregister, },
{ "names", &name_new, },
{ "names", &name_firstupdate, },
{ "names", &name_update, },
Expand Down
Loading