Add name_autoregister RPC #436

yanmaani · 2021-06-12T17:46:27Z

This PR adds the name_autoregister RPC, with delegation support.

Note that there is a bug, which can be triggered by the testing, that causes the transaction queue to release one block later than it should in certain cases.

I've not been able to narrow it down entirely, but I believe it has to do with mempool logic, and not the transaction queue or any code in this PR. Looking at the logs, the transactions do get dequeued and entered into the mempool properly, and the failure comes after that. Nevertheless, there is a chance of about 3% that the tests will fail randomly.

To reproduce consistently, see the line commented # hack. I believe it has to do with generating multiple blocks at a time, so it might not even be possible to reproduce it under IRL conditions.

This bug is not liable to produce any serious user harm, loss of funds, etc, other than a small annoyance. The bug may make name "sniping" less reliable than expected, but that is definitely not a feature of this patch.

Thank you for your consideration.

(Edited to reflect the final PR, that's why the conversation below looks strange)

yanmaani · 2021-06-22T16:00:40Z

@domob1812 Should I split this up into 2 PRs?

domob1812 · 2021-06-23T04:46:06Z

Regarding marking the inputs as unspendable: There is a "lock" functionality in the wallet (for lockunspent), perhaps you could use that. I believe it is not persistent and only stored in memory, but you could read the wallet on startup to look for all those "delayed" transactions and then go and lock the corresponding inputs for them on startup, maybe.

For splitting up the PRs, do you mean the name function refactoring vs name_autoregister? That sounds like a good idea (but I don't see any of that refactoring in the code yet, except for nameExists, which I definitely like).

yanmaani · 2021-06-24T00:17:04Z

@domob1812 Right, I understand I'm supposed to use the LockCoin API, but even if I copy the code more or less verbatim I can't seem to get it to work. Is there something else I need to do for it to work consistently? Something with locking maybe, or to get it to apply the changes?

I've pushed the broken code into the PR to show you what I've tried so you can have a look, but I'm at a loss.

domob1812 · 2021-06-24T04:19:57Z

Unfortunately, I've never used the coin-locking feature myself, so I don't really have code-level experience with it. But I will look at the code and play around with it a bit maybe to see if I can find something.

domob1812 · 2021-06-24T04:22:58Z

src/wallet/rpcnames.cpp

+ // build nfu1 = "d/something points to dd/xxx"
+ // build nfu2 = "d/xxx is the value"
+ // queue nfu1,nfu2
+ // return txid from nn1


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.

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?

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.

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.)

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.

yanmaani · 2021-07-04T19:29:38Z

@domob1812 Any ideas?

befa241 Namecoin / Qt: Add name_firstupdate GUI (Jeremy Rand) Pull request description: This PR adds a GUI for `name_firstupdate`, based on Brandon's port of Mikhail's GUI. AFAICT, once #436 is merged, patching this GUI to use `name_autoregister` instead should be roughly a one-liner. Until then, it's still a UX improvement over the status quo, even though the user needs to run `name_new` themselves. Refs #187 ACKs for top commit: domob1812: ACK befa241. Tree-SHA512: 044e2daf0b67992013703fe3e4e6fe255585fa0268b5b084a92c6eff0027f662891adb9633b1a4c1fef7a39de2e796b8aa713b0f901558ce016a00318e5b602a

yanmaani · 2021-10-18T02:35:45Z

Okay, let's go. This is ready for review.

@domob1812

domob1812 · 2021-10-18T04:04:13Z

I'm currently travelling but will take a look in ten days when I'm back - sorry for the inconvenience.

domob1812

Thanks for your work on this! Unfortunately I think this still needs quite a bit of changes, including a bit of high-level refactoring.

domob1812 · 2021-10-29T07:54:10Z

test/functional/name_autoregister.py

+
+ self.log.info("Register a name.")
+ node.name_autoregister("d/name", "value")
+ assert(len(node.listqueuedtransactions()) == 1)


Please use assert_equal here and at all other places where it is appropriate. This is more explicit when reading the code, and also generates a more explicit error message in case it fails.

domob1812 · 2021-10-29T07:56:32Z

test/functional/name_autoregister.py

+ assert(len(node.listqueuedtransactions()) == 0)
+ node.generate(1)
+ self.checkNameData(node.name_show("d/delegated"), "d/delegated", '{"import":"dd/delegated"}', 30, False)
+ self.checkNameData(node.name_show("dd/delegated"), "dd/delegated", 'value', 30, False)


I'm not convinced that we should do delegation at all yet. But even if we do, I don't really like this style of testing - because it depends on the implementation details of how delegation works. The proper way to write a high-level test like this would be to just look up like a resolver would, and then verify that it ultimately leads to the expected value. We don't care exactly how many transactions are queued or what exactly the delegated name is. All we care about here is that in the end it produces something that works when resolved.

domob1812 · 2021-10-29T07:58:19Z

src/wallet/rpcnames.cpp

 throw JSONRPCError (RPC_TRANSACTION_ERROR,
 "this name can not be updated");
+ CNameData oldData;
+ const auto& coinsTip = chainman.ActiveChainstate ().CoinsTip ();
+ coinsTip.GetName (name, oldData);


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.

domob1812 · 2021-10-29T08:07:39Z

src/wallet/rpcnames.cpp

@@ -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);


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.

domob1812 · 2021-10-29T08:08:54Z

src/wallet/rpcnames.cpp

+ {"value", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Value for the name"},
+ optHelp.buildRpcArg(),
+ },
+ RPCResult {RPCResult::Type::ARR_FIXED, "", "",


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.

domob1812 · 2021-10-29T08:09:57Z

src/wallet/rpcnames.cpp

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


No need for this lock I think.

domob1812 · 2021-10-29T08:10:45Z

src/wallet/rpcnames.cpp

+ throw JSONRPCError(RPC_TRANSACTION_ERROR, "this name exists already");
+ }
+
+ // TODO: farm out to somewhere else for namespace parsing


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.

domob1812 · 2021-10-29T08:13:17Z

src/wallet/rpcnames.cpp

+ the user could have gotten from another RPC command prior to now. */
+ pwallet->BlockUntilSyncedToCurrentChain();
+
+ auto issue_nn = [&] (const valtype name, bool push) {


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.

yanmaani · 2021-10-30T07:48:36Z

Thanks for all your review Daniel!

I agree very strongly with everything you have said about delegation. I think the current situation is really hacky and frankly the code I submitted is not all that good. So I will take that out and save it for later, because it needs to be dealt with more calmly. There's already an issue for that stuff and so on.

I will look over the other stuff you said because it will take some time to fix that. Basically, am I right in thinking that I might as well just fully refactor out the issue_xx stuff and use them in name_new and name_firstupdate as well?

domob1812 · 2021-11-01T05:32:44Z

Thanks! And yes, if you can factor out the issue things and make them reused in the name_new and name_firstupdate RPCs (i.e. as a first step, just factor out the logic you will need to reuse into helper methods from them), that would be great! This could then even be a separate PR to get a headstart in reviewing and merging away some of the changes already.

ghost · 2022-12-25T09:18:24Z

I believe that a pre-register and a register button are sufficient.

Auto registration seems a little far-fetched. The user would have little or no control over the process, and it does not appear that such an auto registration can be cancelled, which could be problematic.

domob1812 reviewed Jun 24, 2021

View reviewed changes

yanmaani mentioned this pull request Jul 8, 2021

RPC: Validate name_anyupdate values #191

Open

JeremyRand added rpc enhancement labels Jul 9, 2021

JeremyRand mentioned this pull request Aug 22, 2021

Add name_firstupdate GUI #443

Merged

yanmaani force-pushed the name-autoregister branch from 8d411a1 to 8fe8c1d Compare October 15, 2021 17:39

yanmaani added 3 commits October 17, 2021 12:00

rpc: Add name_autoregister RPC

2814fad

test: Add integration test for name_autoregister

1428d5f

rpc: Refactor name_update to use new existsName method

1bac03f

yanmaani force-pushed the name-autoregister branch from 8fe8c1d to 1bac03f Compare October 18, 2021 02:31

yanmaani changed the title ~~(WIP) Add name_autoregister RPC~~ Add name_autoregister RPC Oct 18, 2021

yanmaani marked this pull request as ready for review October 18, 2021 02:36

domob1812 requested changes Oct 29, 2021

View reviewed changes

yanmaani mentioned this pull request Oct 30, 2021

RPC for certain users to generate certain data according to standards #474

Open

JeremyRand mentioned this pull request Dec 24, 2022

Switch Buy Names page to use name_autoregister #522

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add name_autoregister RPC #436

Add name_autoregister RPC #436

yanmaani commented Jun 12, 2021 •

edited

yanmaani commented Jun 22, 2021

domob1812 commented Jun 23, 2021

yanmaani commented Jun 24, 2021

domob1812 commented Jun 24, 2021

domob1812 Jun 24, 2021

yanmaani Jun 24, 2021

domob1812 Jun 25, 2021

JeremyRand Jun 25, 2021

yanmaani Jun 25, 2021

yanmaani commented Jul 4, 2021

yanmaani commented Oct 18, 2021

domob1812 commented Oct 18, 2021

domob1812 left a comment

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

domob1812 Oct 29, 2021

yanmaani commented Oct 30, 2021

domob1812 commented Nov 1, 2021

ghost commented Dec 25, 2022

Add name_autoregister RPC #436

Are you sure you want to change the base?

Add name_autoregister RPC #436

Conversation

yanmaani commented Jun 12, 2021 • edited

yanmaani commented Jun 22, 2021

domob1812 commented Jun 23, 2021

yanmaani commented Jun 24, 2021

domob1812 commented Jun 24, 2021

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

yanmaani commented Jul 4, 2021

yanmaani commented Oct 18, 2021

domob1812 commented Oct 18, 2021

domob1812 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

Choose a reason for hiding this comment

yanmaani commented Oct 30, 2021

domob1812 commented Nov 1, 2021

ghost commented Dec 25, 2022

yanmaani commented Jun 12, 2021 •

edited