-
Notifications
You must be signed in to change notification settings - Fork 149
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
|
@@ -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); | ||
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, "", "", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 () | ||
{ | ||
|
@@ -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. */ | ||
|
@@ -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) | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if (isDefaultVal) | ||
value = oldData.getValue(); | ||
outp = oldData.getUpdateOutpoint (); | ||
|
There was a problem hiding this comment.
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 needcs_main
? It seems thatActiveChainstate
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.