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

manage names tab merged against current master #187

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@brandonrobertz

brandonrobertz commented Oct 18, 2017

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

This comment has been minimized.

brandonrobertz commented Oct 18, 2017

@domob1812 domob1812 self-requested a review Oct 18, 2017

@domob1812

This comment has been minimized.

domob1812 commented Oct 18, 2017

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

This comment has been minimized.

brandonrobertz commented Oct 18, 2017

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

Looks very good, I'm just very pedantic -- thanks for your hard work on this!

@@ -6,6 +6,8 @@

#include "script/names.h"

std::map<std::string, NameNewReturn > pendingNameFirstUpdate;

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

Formatting nit: I would remove the space between NameNewReturn and the closing >.

@@ -10,6 +10,8 @@
#include "script/script.h"
#include "serialize.h"

#include <univalue.h>

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

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.


/* Here is where we store our pending name_firstupdates (see above)
while we're waiting for name_new to confirm. */
extern std::map<std::string, NameNewReturn > pendingNameFirstUpdate;

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

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.

@brandonrobertz

brandonrobertz Oct 19, 2017

For this one, what do you think about this refactor strategy:

  • move pendingNameFirstUpdate map to walletdb
  • move reads, writes, exists checks on pendingNameFirstUpdate to methods in walletdb
  • in the UI do the interaction through walletmodel (QT model) which wraps function calls to walletdb

This comment has been minimized.

@domob1812

domob1812 Oct 20, 2017

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

@@ -15,9 +15,12 @@ class BitcoinAddressEntryValidator : public QValidator
Q_OBJECT

public:
explicit BitcoinAddressEntryValidator(QObject *parent);
explicit BitcoinAddressEntryValidator(QObject *parent, bool fAllowEmpty = false);

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

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!


State validate(QString &input, int &pos) const;

private:
bool allowEmpty;

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

Nit: I think you can mark this as const.

NameNewReturn ret;
ret.hex = txid;
ret.rand = rand;
ret.data = pendingData;

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

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.

@brandonrobertz

brandonrobertz Oct 31, 2017

I agree and I'll add this to the list.


BOOST_FIXTURE_TEST_SUITE(wallet_name_pending_tests, WalletTestingSetup)

BOOST_AUTO_TEST_CASE(wallet_name_pending_tests)

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

Thanks for writing a unit test! :)

std::string nameBad = "test/baddata";
std::string txid = "9f73e1dfa3cbae23d008307e42e72beb8c010546ea2a7b9ff32619676a9c64a6";
std::string rand = "092abbca8a938103abcc";
std::string data = "{\"foo\": \"bar\"}";

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

Can all these be const?


// make sure we've added our pending name
BOOST_CHECK(pendingNameFirstUpdate.size() == 1);
BOOST_CHECK(pendingNameFirstUpdate.find(nameGood) != pendingNameFirstUpdate.end());

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

Nit: I personally would write the check as pendingNameFirstUpdate.count(nameGood) > 0, but that's probably just a matter of taste. I find that version easier to read.

BOOST_CHECK(CWalletDB(dbw).EraseNameFirstUpdate(nameBad));
}
}

This comment has been minimized.

@domob1812

domob1812 Oct 18, 2017

Bonus points: Can you also add a unit test for the auto-updating logic? That would be even better to have, I think -- but may be complicated to write (I don't know how unit testing in Qt works).

This comment has been minimized.

@brandonrobertz

brandonrobertz Oct 23, 2017

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.

@domob1812

domob1812 Oct 23, 2017

Yes, I agree. That's certainly not required for merging this IMHO - but if you do find time to work on it later, it would be much appreciated! (And I feel your pain in writing the Qt tests!)

@domob1812

This comment has been minimized.

domob1812 commented Oct 18, 2017

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

@brandonrobertz brandonrobertz force-pushed the brandonrobertz:master branch from 877c11c to 671de5f Oct 23, 2017

@domob1812

This comment has been minimized.

domob1812 commented Oct 23, 2017

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 brandonrobertz force-pushed the brandonrobertz:master branch from 671de5f to 6b61c8a Oct 24, 2017

@brandonrobertz brandonrobertz force-pushed the brandonrobertz:master branch 9 times, most recently from ea6071a to 6f087a9 Oct 31, 2017

@brandonrobertz

This comment has been minimized.

brandonrobertz commented Nov 2, 2017

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

This comment has been minimized.

domob1812 commented Nov 2, 2017

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 brandonrobertz force-pushed the brandonrobertz:master branch 2 times, most recently from 54b6090 to 9f21369 Nov 6, 2017

@brandonrobertz

This comment has been minimized.

brandonrobertz commented Nov 7, 2017

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.

@domob1812

I've added a few more comments, but now everything are just minor things - no bigger refactorings as last time. Thanks for all your work on this!

(But note again that I've not reviewed the Qt-specific stuff too closely, as I don't know much about it anyway. This is particularly true about the .ui files.)

class CNamePendingData
{
private:
std::vector<unsigned char> vchToAddress;

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

Please use vchType as elsewhere in the names/* code.

inline const std::string getToAddress() { return ValtypeToString(vchToAddress); }
inline const std::string getHex() { return ValtypeToString(vchHex); }
inline const std::string getRand() { return ValtypeToString(vchRand); }
inline const std::string getData() { return ValtypeToString(vchData); }

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

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.

@brandonrobertz

brandonrobertz Nov 11, 2017

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.

@brandonrobertz

brandonrobertz Nov 11, 2017

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.

@domob1812

domob1812 Nov 12, 2017

Thanks! Yes, that's exactly what I meant - storing and serialising as raw data. Interfacing to the UI as strings is certainly fine.


explicit ConfigureNameDialog(const PlatformStyle *platformStyle,
const QString &_name, const QString &data,
bool _firstUpdate, QWidget *parent = 0);

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

Nit: nullptr instead of 0.

void on_pasteButton_clicked();

private:
Ui::ConfigureNameDialog *ui;

This comment has been minimized.

@domob1812
QString returnTransferTo;
WalletModel *walletModel;
QString name;
bool firstUpdate;

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

Nit: I think a couple of these variables can actually be const (e. g., firstUpdate).


std::vector<std::reference_wrapper<std::string>> WalletModel::sendPendingNameFirstUpdates()
{
std::vector<std::reference_wrapper<std::string>> successfulNames;

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

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?

const int confirms = val.get_int ();
LogPrintf ("Pending Name FirstUpdate Confirms: %d\n", confirms);

if ((unsigned int)confirms < MIN_FIRSTUPDATE_DEPTH)

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

Nit: Prefer static_cast over C-style casts.

completedResult = this->completePendingNameFirstUpdate(name, rand, txid, data, toaddress);

// Locks wallet
unlock_ctx.reset(nullptr);

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

I think you can just reset() without passing nullptr.

LOCK(cs_wallet);
std::map<std::string, CNamePendingData> pn = namePendingMap;
std::map<std::string, CNamePendingData>::iterator it = pn.find(name);
return it->second;

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

How can your caller know if the name was not found? (Since namePendingMap.end() is presumably private, if you have this access function defined.)

@@ -17,6 +18,7 @@
#include "wallet/wallet.h"

#include <atomic>
#include <univalue.h>

This comment has been minimized.

@domob1812

domob1812 Nov 7, 2017

For what do you need this?

@brandonrobertz brandonrobertz force-pushed the brandonrobertz:master branch 6 times, most recently from 9ad7ae2 to 2f7d103 Nov 8, 2017

@brandonrobertz

This comment has been minimized.

brandonrobertz commented Nov 25, 2017

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

This comment has been minimized.

mkg20001 commented Nov 26, 2017

  1. I was already using namecoin nametab from manage-dns branch (at 8dac7b7). 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

This comment has been minimized.

brandonrobertz commented Nov 27, 2017

@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

This comment has been minimized.

Member

JeremyRand commented Jan 9, 2018

@brandonrobertz needs rebase due to merge conflicts.

@JeremyRand

This comment has been minimized.

Member

JeremyRand commented Jan 16, 2018

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 brandonrobertz force-pushed the brandonrobertz:master branch from 7a456d8 to 4d01a30 Jan 22, 2018

@brandonrobertz brandonrobertz force-pushed the brandonrobertz:master branch from 4d01a30 to f8d7ee6 Jan 22, 2018

@brandonrobertz

This comment has been minimized.

brandonrobertz commented Jan 22, 2018

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.

name, data,
NameTableEntry::NAME_UNCONFIRMED,
"pending registration");
LogPrintf("found pending name: name=%s\n", name.c_str());

This comment has been minimized.

@JeremyRand

JeremyRand Mar 1, 2018

Member

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.

@JeremyRand

This comment has been minimized.

Member

JeremyRand commented Mar 1, 2018

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

} catch (const UniValue& e) {
UniValue message = find_value(e, "message");
std::string errorStr = message.get_str();
LogPrintf ("unexpected name_show response on refreshName=%s: %s\n",

This comment has been minimized.

@JeremyRand

JeremyRand Mar 1, 2018

Member

This spams debug.log with errors while the blockchain is syncing. I suggest checking for the error code RPC_CLIENT_IN_INITIAL_DOWNLOAD and not logging anything in that case.

@JeremyRand

This comment has been minimized.

Member

JeremyRand commented Mar 1, 2018

@brandonrobertz And a bit more review.

@brandonrobertz

This comment has been minimized.

brandonrobertz commented Mar 1, 2018

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

@domob1812

This comment has been minimized.

domob1812 commented Jun 7, 2018

@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

This comment has been minimized.

brandonrobertz commented Jun 8, 2018

@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

This comment has been minimized.

Member

JeremyRand commented Jun 13, 2018

@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

This comment has been minimized.

domob1812 commented Jun 13, 2018

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

This comment has been minimized.

brandonrobertz commented Jun 26, 2018

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

@domob1812

This comment has been minimized.

domob1812 commented Jun 27, 2018

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

@domob1812

This comment has been minimized.

domob1812 commented Jun 27, 2018

Created #233 for tracking the RPC-based automatic name_firstupdate.

@brandonrobertz

This comment has been minimized.

brandonrobertz commented Jun 28, 2018

@domob1812 okay I'm on board with that plan. I can look into the name listing related stuff secondarily. Should I close this PR since we're not going to merge it in its current form?

@domob1812

This comment has been minimized.

domob1812 commented Jun 29, 2018

In the end, we will not merge this PR. But I'm fine if you want to keep it open while we still work on that - up to you.

@JeremyRand

This comment has been minimized.

Member

JeremyRand commented Jun 29, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment