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

qt re-implementation of manage names #67

Merged
merged 21 commits into from Dec 1, 2016

Conversation

Projects
None yet
9 participants
@brandonrobertz

brandonrobertz commented Feb 21, 2016

This is a merge and partial re-write of "manage names" tab from namecoin legacy on top of namecoin-core. I used the RPC functions, internally, to do the heavy lifting. The previous QT UI duplicated much of the code. I also moved the manage names UI from a separate popup to a tab in the main UI.

I tried to keep the patch footprint as small as possible. If anyone sees room to make it even smaller, please comment. A major goal here was to minimize the probability of conflicts in the future.

Some of the code you see here is directly merged from the legacy names page UI. Please let me know if you see problems in it.

Some areas that warrant extra consideration:

  • The changes in src/qt/transactionrecord.cpp were 100% taken from legacy. They might not be necessary.
  • The changes in walletdb.cpp are very important and should have code review.
  • Testing using different QT/GUI config settings would be helpful
std::string toaddress;
std::string hex;
std::string rand;
std::string data;

This comment has been minimized.

@domob1812

domob1812 Feb 21, 2016

I think these fields should not be strings. Rather, toaddress could be a CScript, hex could be a uint256 and others (rand at least) could be valtype.

@domob1812

domob1812 Feb 21, 2016

I think these fields should not be strings. Rather, toaddress could be a CScript, hex could be a uint256 and others (rand at least) could be valtype.

This comment has been minimized.

@brandonrobertz

brandonrobertz Feb 21, 2016

The reason I have them as strings is because the only use for this information is to turn it into a UniValue JSON object and send to RPC name_firstupdate, which needs to be strings. I can change it if it's important.

@brandonrobertz

brandonrobertz Feb 21, 2016

The reason I have them as strings is because the only use for this information is to turn it into a UniValue JSON object and send to RPC name_firstupdate, which needs to be strings. I can change it if it's important.

This comment has been minimized.

@domob1812

domob1812 Feb 22, 2016

I see. I wrote my comment before I saw how you use the RPC calls, and think that this is fine. Maybe add a comment explaining this to the struct?

@domob1812

domob1812 Feb 22, 2016

I see. I wrote my comment before I saw how you use the RPC calls, and think that this is fine. Maybe add a comment explaining this to the struct?

void on_pasteButton_clicked();
private:
Ui::ConfigureNameDialog *ui;

This comment has been minimized.

@domob1812

domob1812 Feb 21, 2016

Could you make this a non-pointer member instead? I'm not very experienced in Qt and do not know if this is a common pattern, but a very brief look at the code suggests that you could do that. This is in general preferrable to pointers and lets you get rid of the destructor. The same may (or may not) be true for the other members as well.

@domob1812

domob1812 Feb 21, 2016

Could you make this a non-pointer member instead? I'm not very experienced in Qt and do not know if this is a common pattern, but a very brief look at the code suggests that you could do that. This is in general preferrable to pointers and lets you get rid of the destructor. The same may (or may not) be true for the other members as well.

private:
const PlatformStyle *platformStyle;
Ui::ManageNamesPage *ui;

This comment has been minimized.

@domob1812

domob1812 Feb 21, 2016

The same comment about making this a non-pointer applies here as well.

@domob1812

domob1812 Feb 21, 2016

The same comment about making this a non-pointer applies here as well.

}
CNameScript nameScript(it->scriptPubKey);
switch (nameScript.getNameOp ())

This comment has been minimized.

@domob1812

domob1812 Feb 21, 2016

Instead of this switch, you can use nameScript.isAnyUpdate().

@domob1812

domob1812 Feb 21, 2016

Instead of this switch, you can use nameScript.isAnyUpdate().

params.push_back (Pair("name", strName));
try {
res = name_show( params, false);

This comment has been minimized.

@domob1812

domob1812 Feb 21, 2016

In general, I like the way you try to avoid code duplication using the RPC interface. In this case, however, I think that just querying the name database directly is not too complicated. You can try to do that instead of calling name_show - unless you prefer to call name_show nevertheless and think this is easier for you. I'll leave this decision to you.

@domob1812

domob1812 Feb 21, 2016

In general, I like the way you try to avoid code duplication using the RPC interface. In this case, however, I think that just querying the name database directly is not too complicated. You can try to do that instead of calling name_show - unless you prefer to call name_show nevertheless and think this is easier for you. I'll leave this decision to you.

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Feb 21, 2016

One more comment: I think it would be really great if there were any unit or regression tests for the pending name_firstupdate logic. (Including that it is gracefully handled if a pending name_firstupdate name is taken by someone else, for instance.) I realise that this is difficult without adding a dedicated RPC command and am not sure what to do here myself... but this is also important QA-wise, and we should discuss it; maybe someone else has an idea.

domob1812 commented Feb 21, 2016

One more comment: I think it would be really great if there were any unit or regression tests for the pending name_firstupdate logic. (Including that it is gracefully handled if a pending name_firstupdate name is taken by someone else, for instance.) I realise that this is difficult without adding a dedicated RPC command and am not sure what to do here myself... but this is also important QA-wise, and we should discuss it; maybe someone else has an idea.

@brandonrobertz

This comment has been minimized.

Show comment
Hide comment
@brandonrobertz

brandonrobertz Feb 21, 2016

Thank you for the commentary @domob1812 . Almost all of the QT code comes from Namecoin legacy, so I'll need to dig in a little deeper to answer some of the open questions (such as why are we using pointer methods so often). I'm also going to add unit tests as I work through your comments.

brandonrobertz commented Feb 21, 2016

Thank you for the commentary @domob1812 . Almost all of the QT code comes from Namecoin legacy, so I'll need to dig in a little deeper to answer some of the open questions (such as why are we using pointer methods so often). I'm also going to add unit tests as I work through your comments.

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Feb 22, 2016

Don't worry. If others ACK the patch as well, I'm also fine with merging it and polishing the Qt later. (But that depends on how you want to do it.) Unit tests would be great where possible, but as I said, I'm not sure what exactly to do there. If you have any ideas, everything you come up with is great!

domob1812 commented Feb 22, 2016

Don't worry. If others ACK the patch as well, I'm also fine with merging it and polishing the Qt later. (But that depends on how you want to do it.) Unit tests would be great where possible, but as I said, I'm not sure what exactly to do there. If you have any ideas, everything you come up with is great!

@fsb4000

This comment has been minimized.

Show comment
Hide comment
@fsb4000

fsb4000 Feb 25, 2016

Thank you for the commit!
I built Windows wallet without issues and I updated name via Configure Name... Works fine.
A small addition:
http://i.imgur.com/3D9XZIM.png
better do right alignment for "expires in" value
similar to classic namecoin-qt: http://i.imgur.com/CJ4vr9R.png

fsb4000 commented Feb 25, 2016

Thank you for the commit!
I built Windows wallet without issues and I updated name via Configure Name... Works fine.
A small addition:
http://i.imgur.com/3D9XZIM.png
better do right alignment for "expires in" value
similar to classic namecoin-qt: http://i.imgur.com/CJ4vr9R.png

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 20, 2016

Member

@randy-waterhouse Have you had a chance to look at this PR?

Member

JeremyRand commented Mar 20, 2016

@randy-waterhouse Have you had a chance to look at this PR?

@randy-waterhouse

This comment has been minimized.

Show comment
Hide comment
@randy-waterhouse

randy-waterhouse Mar 20, 2016

Seems to build and run ok. Several comments regards comparison with 'old' namecoin-qt:

Old had a "Renew Name" button as well as "Configure Name ..." on Manage Names tab.

Configure Name Pop-UP had 4 sub-tabs DNS CONfiguration, IP Configuration, Identity Configuration and Custom Configuration, new has only the simple Custom Config tab.

In the "Transactions" tab when a namecoin tx has taken place they are shown as "Payment to self" in the new and in the old they showed as "Name Operation" and had a nmc address, namecoin logo and details e.g: name_update: id/randy.

In Overview : recent transactions name_update details and etc not shown.

randy-waterhouse commented Mar 20, 2016

Seems to build and run ok. Several comments regards comparison with 'old' namecoin-qt:

Old had a "Renew Name" button as well as "Configure Name ..." on Manage Names tab.

Configure Name Pop-UP had 4 sub-tabs DNS CONfiguration, IP Configuration, Identity Configuration and Custom Configuration, new has only the simple Custom Config tab.

In the "Transactions" tab when a namecoin tx has taken place they are shown as "Payment to self" in the new and in the old they showed as "Name Operation" and had a nmc address, namecoin logo and details e.g: name_update: id/randy.

In Overview : recent transactions name_update details and etc not shown.

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 24, 2016

Member

Thanks for testing @randy-waterhouse.

Configure Name Pop-UP had 4 sub-tabs DNS CONfiguration, IP Configuration, Identity Configuration and Custom Configuration, new has only the simple Custom Config tab.

Given that the DNS/IP/Identity tabs in 0.3.80 were quite buggy and didn't implement the specs particularly well, I'm not sure if it's worth the effort to port them, or if we should just redo those from scratch. Anyone have opinions on this?

On all other points, I agree with @randy-waterhouse that the 0.3.80 behavior is preferable (although I haven't yet tried to reproduce the behavior that he is seeing).

Member

JeremyRand commented Mar 24, 2016

Thanks for testing @randy-waterhouse.

Configure Name Pop-UP had 4 sub-tabs DNS CONfiguration, IP Configuration, Identity Configuration and Custom Configuration, new has only the simple Custom Config tab.

Given that the DNS/IP/Identity tabs in 0.3.80 were quite buggy and didn't implement the specs particularly well, I'm not sure if it's worth the effort to port them, or if we should just redo those from scratch. Anyone have opinions on this?

On all other points, I agree with @randy-waterhouse that the 0.3.80 behavior is preferable (although I haven't yet tried to reproduce the behavior that he is seeing).

@cassiniNMC

This comment has been minimized.

Show comment
Hide comment
@cassiniNMC

cassiniNMC Mar 24, 2016

Which Boost version is recommended? The latest Boost version works flawlessly in namecoin-core. In this PR it leads to the linker error

Undefined symbols for architecture x86_64:
"Params()", referenced from:
CBlockVersion::SetBaseVersion(int) in libnamecoinconsensus_la-pureheader.o

As far as i can remember Boost 1.49 used to be able to solve this problem (back in the 0.3.x Namecoin-Qt days). Boost 1.49 is rather old, though.

cassiniNMC commented Mar 24, 2016

Which Boost version is recommended? The latest Boost version works flawlessly in namecoin-core. In this PR it leads to the linker error

Undefined symbols for architecture x86_64:
"Params()", referenced from:
CBlockVersion::SetBaseVersion(int) in libnamecoinconsensus_la-pureheader.o

As far as i can remember Boost 1.49 used to be able to solve this problem (back in the 0.3.x Namecoin-Qt days). Boost 1.49 is rather old, though.

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand
Member

JeremyRand commented Mar 24, 2016

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 26, 2016

Member

Since we're on a tight schedule for getting nc0.12.0 released, and it's not clear when Bitcoin Core will merge the changes we need for Windows Gitian builds, I'm willing to make a new branch in this repo, which is identical to master except with Gitian+NSIS working properly. The branch might be called "temp-master-gitian-nsis". From there, I can make a 2nd new branch based on temp-master-gitian-nsis, which has this PR merged. From there, I can release Gitian binaries of this PR for both GNU/Linux and Windows. This way, people can test @brandonrobertz's code without having to build from source, and without waiting for Bitcoin Core to merge what we need. However, we won't be able to merge this PR to master until Bitcoin Core resolves my PR.

Any objections to this approach? (I'm particularly interested in @domob1812's opinion.) Please register any NACKs within 48 hours.

Cheers.

Member

JeremyRand commented Mar 26, 2016

Since we're on a tight schedule for getting nc0.12.0 released, and it's not clear when Bitcoin Core will merge the changes we need for Windows Gitian builds, I'm willing to make a new branch in this repo, which is identical to master except with Gitian+NSIS working properly. The branch might be called "temp-master-gitian-nsis". From there, I can make a 2nd new branch based on temp-master-gitian-nsis, which has this PR merged. From there, I can release Gitian binaries of this PR for both GNU/Linux and Windows. This way, people can test @brandonrobertz's code without having to build from source, and without waiting for Bitcoin Core to merge what we need. However, we won't be able to merge this PR to master until Bitcoin Core resolves my PR.

Any objections to this approach? (I'm particularly interested in @domob1812's opinion.) Please register any NACKs within 48 hours.

Cheers.

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Mar 26, 2016

No objections from me, as long as you do not expect me to manage the branches with upstream pulls.

domob1812 commented Mar 26, 2016

No objections from me, as long as you do not expect me to manage the branches with upstream pulls.

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 26, 2016

Member

@domob1812 Yep, that's fine, I'm planning to manage those branches myself. I will probably only rebase them against upstream right before making Gitian releases. (Which might only be once, depending on when upstream merges my NSIS PR, which will make these extra branches obsolete.

Member

JeremyRand commented Mar 26, 2016

@domob1812 Yep, that's fine, I'm planning to manage those branches myself. I will probably only rebase them against upstream right before making Gitian releases. (Which might only be once, depending on when upstream merges my NSIS PR, which will make these extra branches obsolete.

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 26, 2016

Member

I tried to build this for Windows in Gitian. The exact code used is at https://github.com/JeremyRand/namecoin-core/tree/temp-master-gitian-windows-nsis-tarname-namecoin-name-tab . (The only code I added is for renaming Bitcoin to Namecoin in the NSIS script and the Windows Gitian descriptor.) Sadly, the build failed. It appears that namecoind.exe and namecoin-qt.exe were successfully built, but test_namecoin-qt.exe failed to build with a linker error. Below are the last 20 lines of build.log. If needed, I can post the entire contents of build.log on request.

  CXX      qt/qt_libbitcoinqt_a-moc_walletframe.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletmodel.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletview.o
  CXX      qt/qt_libbitcoinqt_a-paymentrequest.pb.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
  CXXLD    namecoind.exe
  CXXLD    test/test_namecoin.exe
  CXXLD    bench/bench_namecoin.exe
  AR       qt/libbitcoinqt.a
  OBJCXXLD qt/namecoin-qt.exe
  CXXLD    qt/test/test_namecoin-qt.exe
qt/qt_namecoin_qt-bitcoin.o:bitcoin.cpp:(.text.startup+0x1e1): undefined reference to `qInitResources_bitcoin()'
collect2: error: ld returned 1 exit status
make[2]: *** [qt/namecoin-qt.exe] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-x86_64-w64-mingw32/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-x86_64-w64-mingw32/src'
make: *** [all-recursive] Error 1

I have not yet tried to build this branch for Linux. I also have not yet tried to build the latest master branch (plus NSIS/Gitian fixes) for Windows. But, I figure it's useful to post this info sooner rather than later, just in case it turns out to be sufficient information to diagnose the problem.

Member

JeremyRand commented Mar 26, 2016

I tried to build this for Windows in Gitian. The exact code used is at https://github.com/JeremyRand/namecoin-core/tree/temp-master-gitian-windows-nsis-tarname-namecoin-name-tab . (The only code I added is for renaming Bitcoin to Namecoin in the NSIS script and the Windows Gitian descriptor.) Sadly, the build failed. It appears that namecoind.exe and namecoin-qt.exe were successfully built, but test_namecoin-qt.exe failed to build with a linker error. Below are the last 20 lines of build.log. If needed, I can post the entire contents of build.log on request.

  CXX      qt/qt_libbitcoinqt_a-moc_walletframe.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletmodel.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletview.o
  CXX      qt/qt_libbitcoinqt_a-paymentrequest.pb.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
  CXXLD    namecoind.exe
  CXXLD    test/test_namecoin.exe
  CXXLD    bench/bench_namecoin.exe
  AR       qt/libbitcoinqt.a
  OBJCXXLD qt/namecoin-qt.exe
  CXXLD    qt/test/test_namecoin-qt.exe
qt/qt_namecoin_qt-bitcoin.o:bitcoin.cpp:(.text.startup+0x1e1): undefined reference to `qInitResources_bitcoin()'
collect2: error: ld returned 1 exit status
make[2]: *** [qt/namecoin-qt.exe] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-x86_64-w64-mingw32/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-x86_64-w64-mingw32/src'
make: *** [all-recursive] Error 1

I have not yet tried to build this branch for Linux. I also have not yet tried to build the latest master branch (plus NSIS/Gitian fixes) for Windows. But, I figure it's useful to post this info sooner rather than later, just in case it turns out to be sufficient information to diagnose the problem.

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Mar 26, 2016

Member

Regarding my previous post, I just tested building the same codebase but without this PR. That branch is at https://github.com/JeremyRand/namecoin-core/tree/temp-master-gitian-windows-nsis-tarname-namecoin . Gitian builds for Windows succeed in that branch, but they don't in the branch I linked in my previous post. That suggests that this PR is responsible for the issue. (Or maybe my build VM is whacked... anyone want to try to reproduce my issue?) @brandonrobertz any idea what might be wrong?

Member

JeremyRand commented Mar 26, 2016

Regarding my previous post, I just tested building the same codebase but without this PR. That branch is at https://github.com/JeremyRand/namecoin-core/tree/temp-master-gitian-windows-nsis-tarname-namecoin . Gitian builds for Windows succeed in that branch, but they don't in the branch I linked in my previous post. That suggests that this PR is responsible for the issue. (Or maybe my build VM is whacked... anyone want to try to reproduce my issue?) @brandonrobertz any idea what might be wrong?

@brandonrobertz

This comment has been minimized.

Show comment
Hide comment
@brandonrobertz

brandonrobertz Mar 26, 2016

@JeremyRand what version of QT are you using when you get that error? That looks like some kind of a QT resource building error. My guess is there's some kind of a bug in the build scripts that gets triggered in this env specifically.

I was able to find a few other instances of this error in previous bitcoin builds (not very helpful):

https://travis-ci.org/bitcoin/bitcoin/jobs/104136463#L2044

Here's a bunch of related problems:

http://www.qtforum.org/article/33015/qinitresources-x-undefined.html?s=1d7109603ae020bb9f384d51e649c605d97d37db#post106346

Solution here was a missing parameter from a build command.

http://stackoverflow.com/questions/17573162/linker-error-in-mdi-qt-application-undefined-reference-to-qinitresources-mdi

http://www.qtcentre.org/archive/index.php/t-10203.html

The solution to these two was a missing resource file.

Seems to be a fairly common thing. Any way I can help?

brandonrobertz commented Mar 26, 2016

@JeremyRand what version of QT are you using when you get that error? That looks like some kind of a QT resource building error. My guess is there's some kind of a bug in the build scripts that gets triggered in this env specifically.

I was able to find a few other instances of this error in previous bitcoin builds (not very helpful):

https://travis-ci.org/bitcoin/bitcoin/jobs/104136463#L2044

Here's a bunch of related problems:

http://www.qtforum.org/article/33015/qinitresources-x-undefined.html?s=1d7109603ae020bb9f384d51e649c605d97d37db#post106346

Solution here was a missing parameter from a build command.

http://stackoverflow.com/questions/17573162/linker-error-in-mdi-qt-application-undefined-reference-to-qinitresources-mdi

http://www.qtcentre.org/archive/index.php/t-10203.html

The solution to these two was a missing resource file.

Seems to be a fairly common thing. Any way I can help?

@JeremyRand

This comment has been minimized.

Show comment
Hide comment

@JeremyRand JeremyRand added this to the nc0.12.0 milestone Apr 2, 2016

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Apr 2, 2016

Member

I rebased the branch that I linked above against a more recent Namecoin Core master branch; it's at https://github.com/JeremyRand/namecoin-core/tree/temp-master-gitian-windows-nsis-tarname-namecoin-name-tab (same URL as before). This time, at @brandonrobertz's suggestion, I tried a Gitian build for Linux. The last screenful of build.log is:

  CXX      qt/qt_libbitcoinqt_a-moc_transactiondescdialog.o
  CXX      qt/qt_libbitcoinqt_a-moc_transactionfilterproxy.o
  CXX      qt/qt_libbitcoinqt_a-moc_transactiontablemodel.o
  CXX      qt/qt_libbitcoinqt_a-moc_transactionview.o
  CXX      qt/qt_libbitcoinqt_a-moc_utilitydialog.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletframe.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletmodel.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletview.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
  CXXLD    namecoind
/home/ubuntu/build/namecoin/depends/i686-pc-linux-gnu/lib/libzmq.a(libzmq_la-ipc_listener.o): In function `zmq::ipc_listener_t::set_address(char const*)':
ipc_listener.cpp:(.text+0x730): warning: the use of `tempnam' is dangerous, better use `mkstemp'
  AR       qt/libbitcoinqt.a
  OBJCXXLD qt/namecoin-qt
  CXXLD    qt/test/test_namecoin-qt
/home/ubuntu/build/namecoin/depends/i686-pc-linux-gnu/lib/libzmq.a(libzmq_la-ipc_listener.o): In function `zmq::ipc_listener_t::set_address(char const*)':
ipc_listener.cpp:(.text+0x730): warning: the use of `tempnam' is dangerous, better use `mkstemp'
/home/ubuntu/build/namecoin/depends/i686-pc-linux-gnu/lib/libzmq.a(libzmq_la-ipc_listener.o): In function `zmq::ipc_listener_t::set_address(char const*)':
ipc_listener.cpp:(.text+0x730): warning: the use of `tempnam' is dangerous, better use `mkstemp'
qt/qt_namecoin_qt-bitcoin.o: In function `main':
bitcoin.cpp:(.text.startup+0x178): undefined reference to `qInitResources_bitcoin()'
collect2: error: ld returned 1 exit status
make[2]: *** [qt/namecoin-qt] Error 1
make[2]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-i686-pc-linux-gnu/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-i686-pc-linux-gnu/src'
make: *** [all-recursive] Error 1

So, it appears that the error occurs when building for both Linux or Windows; it's not specific to a single OS.

Member

JeremyRand commented Apr 2, 2016

I rebased the branch that I linked above against a more recent Namecoin Core master branch; it's at https://github.com/JeremyRand/namecoin-core/tree/temp-master-gitian-windows-nsis-tarname-namecoin-name-tab (same URL as before). This time, at @brandonrobertz's suggestion, I tried a Gitian build for Linux. The last screenful of build.log is:

  CXX      qt/qt_libbitcoinqt_a-moc_transactiondescdialog.o
  CXX      qt/qt_libbitcoinqt_a-moc_transactionfilterproxy.o
  CXX      qt/qt_libbitcoinqt_a-moc_transactiontablemodel.o
  CXX      qt/qt_libbitcoinqt_a-moc_transactionview.o
  CXX      qt/qt_libbitcoinqt_a-moc_utilitydialog.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletframe.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletmodel.o
  CXX      qt/qt_libbitcoinqt_a-moc_walletview.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin.o
  CXX      qt/qt_libbitcoinqt_a-qrc_bitcoin_locale.o
  CXXLD    namecoind
/home/ubuntu/build/namecoin/depends/i686-pc-linux-gnu/lib/libzmq.a(libzmq_la-ipc_listener.o): In function `zmq::ipc_listener_t::set_address(char const*)':
ipc_listener.cpp:(.text+0x730): warning: the use of `tempnam' is dangerous, better use `mkstemp'
  AR       qt/libbitcoinqt.a
  OBJCXXLD qt/namecoin-qt
  CXXLD    qt/test/test_namecoin-qt
/home/ubuntu/build/namecoin/depends/i686-pc-linux-gnu/lib/libzmq.a(libzmq_la-ipc_listener.o): In function `zmq::ipc_listener_t::set_address(char const*)':
ipc_listener.cpp:(.text+0x730): warning: the use of `tempnam' is dangerous, better use `mkstemp'
/home/ubuntu/build/namecoin/depends/i686-pc-linux-gnu/lib/libzmq.a(libzmq_la-ipc_listener.o): In function `zmq::ipc_listener_t::set_address(char const*)':
ipc_listener.cpp:(.text+0x730): warning: the use of `tempnam' is dangerous, better use `mkstemp'
qt/qt_namecoin_qt-bitcoin.o: In function `main':
bitcoin.cpp:(.text.startup+0x178): undefined reference to `qInitResources_bitcoin()'
collect2: error: ld returned 1 exit status
make[2]: *** [qt/namecoin-qt] Error 1
make[2]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-i686-pc-linux-gnu/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/ubuntu/build/namecoin/distsrc-i686-pc-linux-gnu/src'
make: *** [all-recursive] Error 1

So, it appears that the error occurs when building for both Linux or Windows; it's not specific to a single OS.

@brandonrobertz

This comment has been minimized.

Show comment
Hide comment
@brandonrobertz

brandonrobertz Nov 20, 2016

Thanks for the recap Jeremy. I'm going to keep all that around for my next branch of small UI improvements.

brandonrobertz commented Nov 20, 2016

Thanks for the recap Jeremy. I'm going to keep all that around for my next branch of small UI improvements.

@cassiniNMC

This comment has been minimized.

Show comment
Hide comment
@cassiniNMC

cassiniNMC Nov 21, 2016

Did a quick build attempt of https://github.com/namecoin/namecoin-core/archive/nc0.13.99-name-tab-beta1.zip in OSX 10.11.
I have to use protobuf 2.6 because of bitcoin/bitcoin#8741 which resulted in PR bitcoin/bitcoin#8754 (a temporary solution, see laanwj's comment in there).
Now qt_namecoin_qt-bitcoin.cpp doesn't compile anymore:

 CXX      qt/qt_namecoin_qt-bitcoin.o
In file included from qt/bitcoin.cpp:24:
In file included from ./qt/paymentserver.h:35:
In file included from ./qt/paymentrequestplus.h:8:
./qt/paymentrequest.pb.h:12:2: error:
This file was generated by a newer version of protoc which is
incompatible with your Protocol Buffer headers. Please update your headers.
./qt/paymentrequest.pb.h:22:10: fatal error: 'google/protobuf/arena.h' file not found

Is there a workaround?

(#103 may be related.)

cassiniNMC commented Nov 21, 2016

Did a quick build attempt of https://github.com/namecoin/namecoin-core/archive/nc0.13.99-name-tab-beta1.zip in OSX 10.11.
I have to use protobuf 2.6 because of bitcoin/bitcoin#8741 which resulted in PR bitcoin/bitcoin#8754 (a temporary solution, see laanwj's comment in there).
Now qt_namecoin_qt-bitcoin.cpp doesn't compile anymore:

 CXX      qt/qt_namecoin_qt-bitcoin.o
In file included from qt/bitcoin.cpp:24:
In file included from ./qt/paymentserver.h:35:
In file included from ./qt/paymentrequestplus.h:8:
./qt/paymentrequest.pb.h:12:2: error:
This file was generated by a newer version of protoc which is
incompatible with your Protocol Buffer headers. Please update your headers.
./qt/paymentrequest.pb.h:22:10: fatal error: 'google/protobuf/arena.h' file not found

Is there a workaround?

(#103 may be related.)

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Nov 21, 2016

Member

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

cassiniNMC:

Did a quick build attempt of
https://github.com/namecoin/namecoin-core/archive/nc0.13.99-name-
tab-beta1.zip in OSX 10.11. I have to use protobuf 2.6 because of
bitcoin/bitcoin#8741 which resulted in
PR bitcoin/bitcoin#8754 (a temporary
solution, see laanwj's comment in there). Now
qt_namecoin_qt-bitcoin.cpp doesn't compile anymore:

Does this issue also affect the commit in the master branch that
nc0.13.99-name-tab-beta1 is branched from? The error doesn't look at
first glance like it's related to the changes in this PR. You're
using the build instructions from this GitHub repo, right?
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJYM01LAAoJELPy0WV4bWVwgp8QANTh6xEwbQibMTGBjHL9SCzz
YQa482s95GJ8+v/2mzzbC+CuGX4m63e3mnxsIKiEoA+ir98lH/ImgDACb9dLl9+V
fn0LpUZJFCaI5Tu+LbQAjYxTOyHMfPL3apbZCEPmRqvkQJqxMSU1IU2u6slHFGRb
kJUnxh0N7wz7K6gQBiG7w3bKHPL+owgL4SbH3Myb+G++NVdeHH+xSF9Cbtd8h5ie
C5maOqwE7I7vJXQlb5FahLJhs1x/4P8Ve9ZQgbc/4ulZ2MuYdgCdAUqVH4yxJopb
XmvlPd9F47ynorBJEmsBS61m76qEHi+PKOavjfHfPnzvO+1XYLDM2LB5VkwOhMMZ
on9g+2Xx7E4IYPkU18Jnv9VOn+oYnNNl0rB3hYwUKWkurvNqsl3FL2fDlN0RVMiO
plRyEQyd1ImPcGDVO8HMhSpz9tptIU3Zce+32HuieigEMWHFQ6KvDZPwHVxIFWj5
1FkCDjCNIz9VY7l6F0ogOXfzuOzFfciybF/0JTMXyVUklRaIXk0XAGh5nuZrrMTu
0tqrhFaDoixBK/QM5p5K+51ijBw/AlHaRXN86XNTR/nqSy6C/COLhqdLjehhzSUU
/7rjUHpRPIffZabDYF6Jr0khMygEJHWIhx2UrMKw+foHK8lxxmZo51/WmbZBBKJX
1JVda4f5FwEVZM61icao
=+Mfi
-----END PGP SIGNATURE-----

Member

JeremyRand commented Nov 21, 2016

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

cassiniNMC:

Did a quick build attempt of
https://github.com/namecoin/namecoin-core/archive/nc0.13.99-name-
tab-beta1.zip in OSX 10.11. I have to use protobuf 2.6 because of
bitcoin/bitcoin#8741 which resulted in
PR bitcoin/bitcoin#8754 (a temporary
solution, see laanwj's comment in there). Now
qt_namecoin_qt-bitcoin.cpp doesn't compile anymore:

Does this issue also affect the commit in the master branch that
nc0.13.99-name-tab-beta1 is branched from? The error doesn't look at
first glance like it's related to the changes in this PR. You're
using the build instructions from this GitHub repo, right?
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJYM01LAAoJELPy0WV4bWVwgp8QANTh6xEwbQibMTGBjHL9SCzz
YQa482s95GJ8+v/2mzzbC+CuGX4m63e3mnxsIKiEoA+ir98lH/ImgDACb9dLl9+V
fn0LpUZJFCaI5Tu+LbQAjYxTOyHMfPL3apbZCEPmRqvkQJqxMSU1IU2u6slHFGRb
kJUnxh0N7wz7K6gQBiG7w3bKHPL+owgL4SbH3Myb+G++NVdeHH+xSF9Cbtd8h5ie
C5maOqwE7I7vJXQlb5FahLJhs1x/4P8Ve9ZQgbc/4ulZ2MuYdgCdAUqVH4yxJopb
XmvlPd9F47ynorBJEmsBS61m76qEHi+PKOavjfHfPnzvO+1XYLDM2LB5VkwOhMMZ
on9g+2Xx7E4IYPkU18Jnv9VOn+oYnNNl0rB3hYwUKWkurvNqsl3FL2fDlN0RVMiO
plRyEQyd1ImPcGDVO8HMhSpz9tptIU3Zce+32HuieigEMWHFQ6KvDZPwHVxIFWj5
1FkCDjCNIz9VY7l6F0ogOXfzuOzFfciybF/0JTMXyVUklRaIXk0XAGh5nuZrrMTu
0tqrhFaDoixBK/QM5p5K+51ijBw/AlHaRXN86XNTR/nqSy6C/COLhqdLjehhzSUU
/7rjUHpRPIffZabDYF6Jr0khMygEJHWIhx2UrMKw+foHK8lxxmZo51/WmbZBBKJX
1JVda4f5FwEVZM61icao
=+Mfi
-----END PGP SIGNATURE-----

@cassiniNMC

This comment has been minimized.

Show comment
Hide comment
@cassiniNMC

cassiniNMC Nov 21, 2016

Sorry for the fuzz, now everything builds and works perfectly. The problem was caused by a combination of a few stupid mistakes I made using brew and git. In case anyone else runs into this "arena.h not found" error:
Make sure you pay attention to all the warnings and instructions brew comes up with during a brew upgrade or brew install (as in https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md ). And do not forget to do a make clean afterwards (git reset --hard would also do the job but it was bad news for my makeshift git setup).

You're using the build instructions from this GitHub repo, right?

No, I'm not. I'm trying to stick to Bitcoin's build-osx.md for the moment, so I can properly respond to #97 (after some serious testing tomorrow).

cassiniNMC commented Nov 21, 2016

Sorry for the fuzz, now everything builds and works perfectly. The problem was caused by a combination of a few stupid mistakes I made using brew and git. In case anyone else runs into this "arena.h not found" error:
Make sure you pay attention to all the warnings and instructions brew comes up with during a brew upgrade or brew install (as in https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md ). And do not forget to do a make clean afterwards (git reset --hard would also do the job but it was bad news for my makeshift git setup).

You're using the build instructions from this GitHub repo, right?

No, I'm not. I'm trying to stick to Bitcoin's build-osx.md for the moment, so I can properly respond to #97 (after some serious testing tomorrow).

@cassiniNMC

This comment has been minimized.

Show comment
Hide comment
@cassiniNMC

cassiniNMC Nov 25, 2016

I've done a lot of tests during the last few days, and I must say everything looks extremely good! No irregularities whatsoever. There is only a minor issue with the OSX menu bar and the Dock menu (both outside the main window). A click at "Help" or "Settings" has no effect in some situations, for example. This is certainly a non-blocker as there is always an alternative way of opening the desired subwindow.

BTW (thinking of the release notes), we should keep in mind that here and in Namecoin Core the SCRIPT_VERIFY_LOW_S flag is currently deactivated for backward compatibility. See 9f71f45

cassiniNMC commented Nov 25, 2016

I've done a lot of tests during the last few days, and I must say everything looks extremely good! No irregularities whatsoever. There is only a minor issue with the OSX menu bar and the Dock menu (both outside the main window). A click at "Help" or "Settings" has no effect in some situations, for example. This is certainly a non-blocker as there is always an alternative way of opening the desired subwindow.

BTW (thinking of the release notes), we should keep in mind that here and in Namecoin Core the SCRIPT_VERIFY_LOW_S flag is currently deactivated for backward compatibility. See 9f71f45

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Dec 1, 2016

Member
Member

JeremyRand commented Dec 1, 2016

@JeremyRand JeremyRand merged commit 42560a9 into namecoin:master Dec 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

JeremyRand added a commit that referenced this pull request Dec 1, 2016

Merge #67: qt re-implementation of manage names
42560a9 fix broken QT4 compatability (Brandon Roberts)
60000ea fix broken links in names pages (Brandon Roberts)
3e4ae0f prompt user on pending name_firstupdate error (Brandon Roberts)
b19c20b fix iCCP error messages (Brandon Roberts)
4e3efd4 group name pending tests with wallet tests (Brandon Roberts)
890cf1c pass firstupdate strs by reference (Brandon Roberts)
5a75bb6 trigger wallet unlock on name_firstupdate if necessary (Brandon Roberts)
d9d7f1b fixed failing test (Brandon Roberts)
c2638ed fix rebase error (Brandon Roberts)
62c9db9 pending_firstupdate wallet unit tests (Brandon Roberts)
5c83f94 fix transaction record address rendering (Brandon Roberts)
ac30aa5 transaction list, namecoin specific rows (Brandon Roberts)
0de7430 QT: fix scope error, managenames dialog to widget (Brandon Roberts)
63474f7 renew name button/functionality (Brandon Roberts)
fd65fce correct internal jsonrpc error handling (Brandon Roberts)
4f6eea5 show name_new error messages to users (Brandon Roberts)
d9ad122 small ui/typo changes (Brandon Roberts)
d9d30a3 add nameop to qt build (Brandon Roberts)
42aab4e add real nameop icon (Brandon Roberts)
fc253dc start working through pr comments (Brandon Roberts)
1f141c7 qt re-implementation of manage names (Brandon Roberts)
@randy-waterhouse

This comment has been minimized.

Show comment
Hide comment
@randy-waterhouse

randy-waterhouse Dec 1, 2016

Well done to all involved. What's the timeline on getting some 0.13.XX with Managed Names tab binaries out there?

randy-waterhouse commented Dec 1, 2016

Well done to all involved. What's the timeline on getting some 0.13.XX with Managed Names tab binaries out there?

@brandonrobertz

This comment has been minimized.

Show comment
Hide comment
@brandonrobertz

brandonrobertz Dec 2, 2016

@randy-waterhouse there are binaries available here https://namecoin.org/download/betas/ under the "0.13.99-name-tab-beta1" title

brandonrobertz commented Dec 2, 2016

@randy-waterhouse there are binaries available here https://namecoin.org/download/betas/ under the "0.13.99-name-tab-beta1" title

@randy-waterhouse

This comment has been minimized.

Show comment
Hide comment
@randy-waterhouse

randy-waterhouse Sep 4, 2017

Any timeline for when this mod will be added back into the master branch?

randy-waterhouse commented Sep 4, 2017

Any timeline for when this mod will be added back into the master branch?

@brandonrobertz

This comment has been minimized.

Show comment
Hide comment
@brandonrobertz

brandonrobertz Sep 4, 2017

I'm working through the merge. It's mostly done but there are a few strange bugs related to the major upstream overhaul of the wallet internals. I expect to have a new PR available within a week or two.

brandonrobertz commented Sep 4, 2017

I'm working through the merge. It's mostly done but there are a few strange bugs related to the major upstream overhaul of the wallet internals. I expect to have a new PR available within a week or two.

@JeremyRand

This comment has been minimized.

Show comment
Hide comment
@JeremyRand

JeremyRand Sep 5, 2017

Member

@randy-waterhouse @brandonrobertz Please use #134 for discussing porting this PR to master branch. Cheers. :)

Member

JeremyRand commented Sep 5, 2017

@randy-waterhouse @brandonrobertz Please use #134 for discussing porting this PR to master branch. Cheers. :)

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