manage names tab merged against current master

[brandonrobertz]

I've finally completed the merge to namecoin-core/master. I had to refactor several things, due to upstream changed in the internal APIs, so there may be some bugs that didn't exist in the previous version. I also fixed the following bugs:

• pending registration bug, where the client requests unlock over and over and then errors with name registered
• names not showing up in the manage names list properly until client has been restarted

I know domob said he had some stylistic critiques, so I'd like to work through some of those as well once I know what they are.

[brandonrobertz]

Paging @randy-waterhouse and @domob1812

[domob1812]

Thanks a lot for finishing this work - this will be great once launched!

I'll take a look at the code now - and am already sorry for probably a lot of pedantic comments I'll have. ;)

BTW, in the future, I think you should try to have fewer (or just one) commit in a PR - git rebase is your friend here. I think that the code history of a project is important, and having only few commits of logically coherent changes helps a lot (rather than a long list of commits as the code evolved in your local branch). I don't expect you to rebase or squash this PR now, though.

[brandonrobertz]

I know it's annoying to go through a million commits. I can squash it before we merge, if that's OK. Thanks for looking.

[domob1812]

I would prefer to squash the commits if you don't mind -- but I'm also fine with keeping it as it is, if that would be a lot of extra work and complication for you. (Just wanted to point this out for the future.)

[domob1812]

Thanks for the updates! Please let me know when you are ready for a second round of review - and please note that I'll be busy the next couple of weekends, so I can't promise how much time I'll have for Namecoin-related work. But I should probably also be able to do a follow-up review in a couple of evenings during the week.

[brandonrobertz]

Alright. I've worked through all your comments @domob1812 and made lots of improvements. Your review comments were extremely helpful, so thanks for going through everything. Let me know if you get a chance to go through my revisions.

One thing I noticed is that the first test (https://travis-ci.org/namecoin/namecoin-core/jobs/295544697) seems to always fail in Travis, but it succeeds local. Something about not being able to find certain Namecoin commands? I've fixed all the other test issues, though. Maybe @JeremyRand knows more about this?

[domob1812]

Thanks, I'll take another look - although I won't have time until next Monday to do so, unfortunately. In the meantime, @JeremyRand and others: feel free to take a look also, and to test.

[brandonrobertz]

I replaced the JSON serialization with a more standard CNamePendingData serializable class. Note that this change is incompatible with any old beta wallets that have old-style pending names in them.

[brandonrobertz]

I'm trying to add some qt tests and I found something odd. Inside the testing environment, only the following name rpc commands are available:

== Namecoin ==
name_checkdb
name_filter ("regexp" ("maxage" ("from\" ("nb" ("stat")))))
name_history "name"
name_pending ("name")
name_scan ("start" ("count"))
name_show "name"

Does this look like a symptom of something that you recognize, @domob1812 ? The wallet is enabled (these tests will only run when the wallet is enabled), so it's not that. Maybe there's a flag I need to set on the initialization of the wallet or bitcoin itself?

Edit: Upon further investigation I'm getting this error: "Method not found (wallet method is disabled because no wallet is loaded)"

Edit 2: I figured it out. I had to manually add my test wallet (created by the test suite) to the vpwallets map. The Qt testsuite appears to be out-of-date with current Bitcoin wallet handling.

[domob1812]

No, I'm not aware of anything special about the test setup. That said, I've never used the RPC methods in tests except for the Python regtests (which are entirely different). So perhaps one has to add them to some other RPC table or so. If you find something, please let me know - otherwise I may try to look for it later when I have time.

[JeremyRand]

@brandonrobertz:

One thing I noticed is that the first test (https://travis-ci.org/namecoin/namecoin-core/jobs/295544697) seems to always fail in Travis, but it succeeds local. Something about not being able to find certain Namecoin commands? I've fixed all the other test issues, though. Maybe @JeremyRand knows more about this?

The error in contrib/devtools/check-rpc-mappings.py is also happening in master branch, so it's not anything related to this PR. Someone should look at that in another ticket.

[brandonrobertz]

Just an FYI: I've created some Qt unit tests. But what I've pushed is as far as I can go without refactoring NameTableModel so for now it is what it is.

[JeremyRand]

Reviewing cc1d2af330ca2275868cca0e960f1c531d736ed2 merged into 3cdcfe7df8b7565171b4a53b13729c261378227d:

• "Expires in" column in "Manage Names" tab doesn't mention whether a name operation is currently pending for the name. E.g. if you renew a name, nothing changes in the table until the next block is mined, which can be confusing. Would be nice if the "Expires in" column appended " (update pending)" to the number in such cases.
• "Configure Name..." dialog has no obvious way to cancel the name_update. (The non-obvious way is to click the X in the upper-right corner, which works fine.) I suspect that this is an unintentional side effect of allowing the name_firstupdate value to be empty, which removed that use case for the "Cancel" button.
• Issuing a name_update via the "Configure Name..." button while the wallet is locked yields this error after entering the wallet passphrase:

  Unable to update name.
  Reason: Error: Please enter the wallet passphrase with walletpassphrase first.
  

• Working properly AFAICT: renewing names and registering names (although I haven't tried anything particularly exotic).

[brandonrobertz]

@JeremyRand thanks for testing. I'm working through the issues. As for the blank expiration, I plan on having a "status" column that says if the name is pending firstupdate, pending confirm, confirmed, etc. That way I can keep the expires in field an integer.

[brandonrobertz]

I did a major revision to the NameTableModel. I've added "Name Status" to the rows, so the user is informed of the status of the name, in addition to the expiration. I'll need to do a lot more testing, but I think this UI works a lot better given the restrictions imposed by the RPC API.

[mkg20001]

When rescanning the gui crashes after 2017-11-25 10:10:48 unexpected name_show response on refreshName=d/mkg20001: Namecoin is downloading blocks...

[brandonrobertz]

When rescanning the gui crashes after 2017-11-25 10:10:48 unexpected name_show response on refreshName=d/mkg20001: Namecoin is downloading blocks... Thank you for the report, @mkg20001.

I'm investigating this issue now. My initial guess is that the log message is unrelated to the crash. Can you give me the following information:

1. What wallet version did you upgrade from when you re-scanned?
2. Were you using regtest? Mainnet chain? Etc. I need to know what chain you were re scanning and what options you started namecoin-qt with.
3. Did you have any pending name_firstupdates in your wallet when you upgraded versions?

Any other information could help, as well, as I am unable to reproduce the issue.

[mkg20001]

1. I was already using namecoin nametab from manage-dns branch (at 8dac7b7f8bcc8b0d554518e2e57afefea585edbe). Started it, shut it down then run it with -rescan -reindex just so I could find out which names expired through the log.
2. Mainnet
3. No

log.txt OS: Ubuntu 16.04 64bit, AMD CPU CLI parameters: ./src/qt/namecoin-qt -reindex -rescan -addnode=136.243.31.32

[brandonrobertz]

@mkg20001 note that everything in the manage-dns branch is experimental and has nothing to do with this PR. We'll have another PR for that code once it nears completion. Does namecoin-qt still crash when you use 7a456d8 ? (Current master branch)

[JeremyRand]

@brandonrobertz needs rebase due to merge conflicts.

[JeremyRand]

Testing against nc0.15.99-name-tab-beta1:

name_update operations display as "update pending" status until they have 12 confirmations, at which point they change to "confirmed". I think the "update pending" status should reflect whether the transaction has yet been mined, and if so, how many block confirmations exist.

Name registrations' status displays as "pending confirm" when the name_new is not yet mined, which is unclearly worded. A better wording might be "pending registration".

Name registrations completely disappear from the name list when the name_new is mined but the name_firstupdate is not yet issued.

Name registrations show an "expires in" value of 4294967280 when the name_firstupdate is issued but not yet mined. (I suspect this may be a result of accessing uninitialized memory.)

"Configure Name..." dialog has no obvious way to cancel the name_update.

This is still an issue.

Issuing a name_update via the "Configure Name..." button while the wallet is locked yields this error after entering the wallet passphrase:

This is now fixed.

[brandonrobertz]

f8d7ee6 fixes everything in @JeremyRand's review except for the 4294967280 issue, which I have yet to replicate (still working on that) and the disappearing name issue between name_firstupdate sent and mined (will be fixed pending a major name table refactor or API improvement). Latest push also is rebased on top of most recent upstream.

[JeremyRand]

@brandonrobertz See the review I just posted. If the bug I think I see is actually a bug, it's definitely a blocker.

[JeremyRand]

@brandonrobertz And a bit more review.

[brandonrobertz]

@JeremyRand thanks for the review. I can push fixes for this shortly.

[domob1812]

@brandonrobertz: What is the status here?

As this is a huge project, I'm wondering how we can best proceed. I don't think it makes sense to keep a big patch separate from the mainline code "forever" while fixing the remaining bugs - this just leads to continuous rebasing and an "always broken" branch.

Do you think it would be possible to split the giant patch into smaller, logically coherent pieces so that we can merge them? For instance, small refactorings here and there could be done and merged in independent patches, to at least get rid of them in the main patch. (I've not looked at the code to decide what would be candidates for this - if you want me to make suggestions, let me know and I'd be happy to take a look.) Honestly, I think splitting this into smaller pieces is the only way to really get it through.

[brandonrobertz]

@domob1812 Sorry, but I haven't been able to work on this in a while due to work circumstances. I'm finally getting to a point where I could slot some of this work in.

I agree with your assessment that this gigantic patch has kind of become a beast. I'm not really sure how it would best be split into smaller patches, though. A lot of this stuff is all inter-tangled.

I seem to have caused some issues with some of the more recent changes like the "status" column in the table and possibly some of the refactors for style, etc.

If you have any suggestions on how to move forward with this, I'd be interested in hearing it.

[JeremyRand]

@brandonrobertz I think a lot of the backend code can probably be split out into separate PR's, preferably exposed as RPC methods (which also would allow CLI users to access them). As a starting point, maybe create a PR that adds 2 RPC methods, queue_name_firstupdate and process_transaction_queue. (Exact names are open to debate.) The former would store a name_firstupdate in the wallet, and the latter would broadcast any of the queued transactions if the block height has advanced enough (and if the wallet is unlocked). Thoughts?

[domob1812]

Sorry for not getting back to you earlier. I agree with @JeremyRand - in general, most of the backend logic, helper routines and refactorings necessary can be done in separate PRs.

In particular, I like Jeremy's suggestion of supporting name-firstupdate data in the wallet through the RPC interface. That would be useful by itself and enable better testing in addition to making this PR easier to handle.

If you add RPCs as suggested by Jeremy, they should go into src/wallet/rpcnames.cpp. Ideally, there should also be corresponding regtests in new name_*.py files inside test/functional. Let me know if you need guidance for the regtests, or if you prefer me to add those after you add the implementation.

[brandonrobertz]

@domob1812 @JeremyRand I think adding the queueing stuff as a RPC command makes the most sense. I can start working around this.

One other major issue with this PR is the lack of a uniform "list all names command". I'm having to pull names using several different RPC commands and even then there are holes where names won't show up. I've talked about this issue here: https://github.com/namecoin/namecoin-core/issues/194#issuecomment-359302067 but it's a significant source of bugs and complexity in the PR.