Remove IP transaction check #8546
Conversation
ghost
mentioned this pull request
|
Please also get rid of the other |
|
@MarcoFalke Done. |
|
Looks like it is still here: https://github.com/mcccs/bitcoin/blob/e05969720213c4b7c7db2115fe0cf96d5ae73e21/src/qt/transactiondesc.cpp#L93 |
maflcko
changed the title
|
Did I remove the wrong one? I removed two |
|
Concept ACK, but this needs review and tests with actual "online" transactions (IP transactions). |
|
@MarcoFalke I cannot find the binaries for v0.7.2 and compiling is difficult. If you give me its link, I can test the Pull Request. |
|
I think IP transaction support was disable way before that. You probably
need 0.3.something...
|
|
|
Removed, yes. But I think it was disabled long before.
|
|
Yes, you were right. The last version it exists is 4.0rc2, thanks for reminding me. |
|
utACK 8fd27c6 |
| @@ -167,9 +146,6 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, TransactionReco | |||
|
|
|||
| if (fAllFromMe) | |||
| { | |||
| if(fAllFromMe & ISMINE_WATCH_ONLY) | |||
| strHTML += "<b>" + tr("From") + ":</b> " + tr("watch-only") + "<br>"; | |||
There was a problem hiding this comment.
Now that this no longer uses From:, do we still have a way to indicate in the transation details that a transaction is watch-only?
There was a problem hiding this comment.
There was a problem hiding this comment.
There's an eye-icon, but that is shown in the transaction list, not in the transaction details.
There was a problem hiding this comment.
Please leave a "Watch-only" notice in the transaction details.
There was a problem hiding this comment.
Yes, but word it differently please. There is no need to display this as From: watch-only. But it needs to be visible that the transaction is watch-only.
There was a problem hiding this comment.
Also isn't this part of code inefficient?
if (fAllFromMe)
{
if(fAllFromMe & ISMINE_WATCH_ONLY)I am going to replace this with this:
if (fAllFromMe)
{
if(ISMINE_WATCH_ONLY)Am I right or should I keep this?
There was a problem hiding this comment.
Eh, no don't do that. Firstly, don't optimize things which are not a bottleneck - one comparison is negligible to all the bit blitting and pixel pushing happening in Qt itself.
It's also far from equivalent: & is a bitwise AND operator, its matching a bit field.
|
Regardless of testing this pull, an old wallet.dat would be helpful for general compatibility testing, too. |
|
@MarcoFalke If you need, you can use this wallet.dat.zip which I just created (created by v0.8.6). EDIT: Wait for a newer wallet.dat. This old one has no transactions. |
|
Does it include IP-transactions? I could assume you need at least two
nodes to create such.
|
|
Nope, not yet. I couldn't find 0.3.24's compiled version, and I'll compile it by myself after I install wxWidgets. |
Do you really need the GUI? was there no way to do IP transactions through bitcoind? I don't remember, could well be true. |
|
If actually testing with such an old version is a problem, maybe it would be possible to emulate such a transaction, e.g. take the code and artificially inject it into the wallet? That would make a good test. |
|
I really don't know if we need to do that much effort for a feature that has been disabled by default since september 30th 2010 (almost 6 years ago). Sure, it shouldn't crash if anyone has records left from such transactions in their wallet, but I also don't think we should show anything useful about them. |
|
I agree, the only thing to check here is that it doesn't crash. |
|
This happened after I entered the command below. What might be causing it? make -f makefile.unix
g++ -c -O2 -Wno-invalid-offsetof -Wformat -g -D__WXDEBUG__ -DNOPCH -DFOURWAYSSE2 -DUSE_SSL -DUSE_UPNP=0 -I/usr/local/lib/wx/include/gtk2-unicode-static-3.1 -I/usr/local/include/wx-3.1 -D_FILE_OFFSET_BITS=64 -D__WXGTK__ -pthread -DGUI -o obj/util.o util.cpp
In file included from util.cpp:4:0:
headers.h:43:20: fatal error: db_cxx.h: No such file or directory
compilation terminated.
|
|
You're missing libdb++.
|
|
Try |
|
Does bitcoind have IP transaction support? net.cpp: In function ‘void ThreadMapPort2(void*)’:
net.cpp:1076:63: error: too few arguments to function ‘UPNPDev* upnpDiscover(int, const char*, const char*, int, int, int*)’
devlist = upnpDiscover(2000, multicastif, minissdpdpath, 0);
(There's an ^ showing 0)
In file included from net.cpp:23:0:
/usr/include/miniupnpc/miniupnpc.h:58:1: note: declared here
upnpDiscover(int delay, const char * multicastif,
(There's an ^ showing upnpDiscovery)
net.cpp:1090:58: error: too few arguments to function ‘int UPNP_AddPortMapping(const char*, const char*, const char*, const char*, const char*, const char*, const char*, const char*, const char*)’
port, port, lanaddr, 0, "TCP", 0);
There's an ^ showing the last 0. |
|
miniupnpc breaks APIs often. Try Note that's a null value for USE_UPNP. (I don't know anything about IP transactions...) |
|
Reviewing historical code, it looks like you need to go back to v0.3.20 to receive IP transactions. Note also that "to" is still supported today by RPC sendtoaddress's comment-to... |
|
Oh, and the receiving IP txns looks like it's part of the GUI... I don't think I ever managed to build the wx GUI, so I can't help with that. :( |
|
https://gist.github.com/mcccs/5b43d4638e303a6b0556b865cfd1f264 Maybe we should close this Pull Request, because it needs too much work for non-important feature. |
|
Simulating an IP transaction may very well be easier. But if you've reduced it to link errors like those, it's probably just a matter of getting the LDFLAGS right now... |
Assembler messages:
Fatal error: can't create obj/nogui/util.o: No such file or directory
makefile.unix:69: recipe for target 'obj/nogui/util.o' failed
make: *** [obj/nogui/util.o] Error 1@luke-jr What am I missing now? |
|
What's the status here? |
|
I'm closing this for now. I'm not convinced that this only removes code related to pay-to-IP transactions, and seems unfeasible to test. Doesn't hurt to leave this code around for a little longer. |
The old Pull Request was junk and it's moved.