Conversation

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.

This comment has been minimized.

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.

This comment has been minimized.

Why do you need this include? I don't see anything in your additions that would require it - or is it required for something existing in the file and currently missing? Please either remove or briefly reply here with the explanation why you need the include.

This comment has been minimized.

If I understand your code correctly, you need this here rather than just in the UI code because it is part of the wallet, right? In that case, however, it should also be stored in the wallet and not in a global variable -- especially in light of the recent upstream changes to support multiple wallets in a single running client.

This comment has been minimized.

Yes, basically. I think you should add the variable to CWallet, where for instance also mapWallet is (since it is very similar in spirit, IMHO). Access should be through CWallet, not walletdb, though. (The walletdb is the backing BDB, but from there you load it into memory just like you do now - except that you keep it in CWallet and ideally do all access to it through the wallet. Do not access walletdb directly, let the wallet do it; at least that's how I think it works for transactions.)

I don't know how the UI typically interacts with the wallet, but I guess walletmodel is the correct way, yes. (In general, I think you should just follow how the wallet transactions are stored and handled, for instance.)

This comment has been minimized.

Why do you need this change? I've not yet finished reading your code, but my guess is that you want an "allow empty" address field for name updates - is that correct, or do you need it for something else? (Apart from that, though, I have no idea how this could be related to adding the name UI.) Please briefly explain here for my understanding. Thanks!

This comment has been minimized.

This has me wonder about the serialisation of NameNewReturn. You could actually define "proper" serialisation of the struct in the same way that other stuff in the code is serialised (e. g., transactions). That would fit better into the existing code here than using JSON.

This comment has been minimized.

Writing Qt unit tests is pretty complicated, but it's been on my list for a while now. I tried a while back but couldn't get it to work correctly basing from the existing Qt tests. I'll definitely give it another stab, though.

This comment has been minimized.

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

This comment has been minimized.

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.

This comment has been minimized.

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.

This comment has been minimized.

If I understand the code correctly, you are actually storing the address in its string representation in the vchType. Similarly for hex and rand, which are stored as hex strings, right?

I think since you have this wrapper class and already need to convert between string and vchType, you should actually encode the data properly: The address can be a CScript, and hex/rand can be the actual binary data in vchType, not the hex string.

This comment has been minimized.

I'm down to this last one before I start doing more testing, but I'm a little confused. Do you want me to not return std::string and instead return standard internal types like CScript etc? Or are you saying I can leave it accepting/returning std::string but encode internally and serialize to the reference types (CScript, etc)?

This comment has been minimized.

I've got it working the first way, accepting/returning std::string but serializing toAddress, txid, rand to CScript, uint256, uint160 respectively. If you had something different in mind, let me know. For the way the UI code is using the wallet, it's easier for me to just use strings and it allows me to not have to mess with internal types in the Qt code. But I can change if required.

This comment has been minimized.

Do you really need a reference_wrapper<string> here? I think just having a vector<string> would make the code simpler and easier to read, besides avoiding any potential issues with references to strings that go out of scope. Of course, a vector of references is smaller and faster, but is that really important here vs code simplicity and clarity?

This comment has been minimized.

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

This comment has been minimized.

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 comment has been minimized.

edited

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.

This comment has been minimized.

I haven't explicitly tested this, but based on code review I suspect that this code will list all name_anyupdate transactions in the mempool, not just name_anyupdate transactions that belong to the user. Checking the "ismine" field for each transaction returned by name_pending would fix that. This is probably related to the behavior reported by exmachinalibertas.

This comment has been minimized.

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.

This comment has been minimized.

@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?

This comment has been minimized.

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.

This comment has been minimized.

edited

@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: #194 (comment) but it's a significant source of bugs and complexity in the PR.

This comment has been minimized.

Yes, I think implementing "just" the "name-firstupdate queued in wallet" thing in the core daemon and with RPC methods is the best next step. As I already said, that will then also allow us to write proper tests for it, and this will hopefully lead to fewer edge-case bugs going forward.

Regarding the "list all names" thing: In principle, name_list does that. The only thing missing that I'm aware of are name_new's that have not yet been first-updated. Is that what you miss, or something else in addition? We could certainly add those - but I'm not sure how useful it would be. For the UI, I would imagine that you instead list the queued firstupdates after implementing them as discussed. We could add those to name_list. (The problem with name_new is that the transaction itself does not contain the name, only a hash. So to list the name, we would have to store it somewhere in the wallet - and that's basically what the queued firstupdate already is, so we should just use that instead.)

This comment has been minimized.

Personally I'd like this PR to remain open until it's been replaced by new PR's that contain equivalent functionality. Even though it won't be merged in its existing form, it's a decent way to coordinate/track the relevant efforts. I don't feel incredibly strongly about it though.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.