0.0.9 UI

[zathras-crypto]

Contains various bits that got layered - sorry about that all. I have one issue that I still haven't been able to narrow down, and that's randomly the GUI will not display at startup after init maybe one out of five times - can anyone replicate?

[dexX7]

136 commits, 49 files changed.. >_<"

Just FYI: there is git rebase -i HEAD~5 (or another number than 5) to edit the last 5 commits. Replacing pick with s can be used to combine two or more commits. r can be used to rename and edit the commit description. Adding a # at the beginning of a line can be used to exclude a commit. git push myrepo -f mybranch can be used to overwrite an already published PR or code.

Rants aside, I'm eager to test this! :)

[dexX7]

@marv-engine: is there a test plan for STO?

[m21]

Sure @dexX7 , right here: https://github.com/marv-engine/QA/blob/testreqs/TestRequirements/SendToOwners_TestReqs.md

[dexX7]

@m21: wey, awesome! I'm well aware of the meta DEx test plan which I already codified mostly (but based on some wrong values with similar functionality though - it should be considered as incomplete). I also stumbled upon the test plan for SP. Are there others?

Just as a side note and FYI: I'll try to adopt @msgilligan's much better test framework (I love those descriptive and telling tests!), but for now my RPC tests are the most complete tests for MC that are out there.

Edit: I was looking for a list of transactions and results, so to speak, not the general "test overview".

[dexX7]

@zathras-crypto: I can confirm: I merged this into current master, but the UI isn't showing up after startup on Ubuntu 14.04 x64. Clicking on the icon in the task bar is without effect, even a right click isn't possible.

Edit: Shutdown via mastercore-cli stop also freezes and does not finish. Edit 2: it starts on second startup.

Edit 3: oh wow, I love the "show transaction details".

[dexX7]

So the intend seems to be to exclude all meta DEx things (my tests also clearly fail when trying to use the RPC commands), but they are not excluded from the parsing chain and show up in my trade history. They also affect my balance. Basically means: I can use the meta DEx via raw transactions and can get away with it. @m21. Is excluding this still WIP?

[m21]

We are only talking about TestNet & TestMSC on MainNet here, right @dexX7 ? (isTransactionTypeAllowed should be invalidating them for MainNet). I don't expect disabling parsing code for TestNet - everything is always allowed on TestNet and this code has been in there for a while now anyway.

[dexX7]

@m21: Ah, yes, I was referring to testnet. This should work.

@zathras-crypto:

1. It would be awesome, if there were "copy address" or "lookup address" in the balances tab.
2. I suggest to hide "crowdsale information", when not applicable. There is no very strong relation between crowdsales and properties per se, so I wouldn't present it, as if this were the case.
3. Any chance to do a search in the "Lookup Property" tab based on partial information? Say for example I type just a few characters of an address and the result includes all properties of all addresses that have that substring.
4. In my opinion a wildcard search in "Lookup Property" would be very useful: pass in nothing or "*" to get every property. This is sort of related to point 3, because "" is a substring or partial information as well.
5. "Lookup transaction" creates a modal. Would be nicer, if the content is printed on the main window instead of in a popup.
6. "Lookup transaction" appears to fire "transaction ID entered was not valid" for anything that is no Mastercoin transaction. Easy: more gentle message "No information found for this transaction hash", more complex: check, if it's a valid transaction, then fire an error, if it's none, and if it's just not a Mastercoin transaction, show a warning that it's no Mastercoin transaction. Or in short: a non Mastercoin transaction can still have a valid transaction id.

[dexX7]

One more: there are also freezes during runtime. Left it open for an hour or so without interaction (but no standby or alike in the meantime), came back and the Qt window is unresponsive. Icon interaction works, but CLI/RPC is also dead - it does not respond with an error, but simply does nothing and probably times out after a while. Nothing unexpected in the logs. It can only be killed, not shutdown:

[m21]

Thanks Dexx; no way am I merging before all got cleared up, worried about this much in one blob :(

[dexX7]

A few more details regarding the freeze: I created 15 properties (SpamTokenN) via sendraw_tx. A few of them confirmed and were created, but not all. The others show up as conflicting transactions in the GUI. When trying to fetch some information about those transactions (getrawtransaction), nothing is available. My guess is that the freeze happend while some of them were still in the mempool. SpamToken1, 2, 3 and 15 confirmed, 4-14 not. They were created in sequence, but it's not unusual that transactions get mined out of order.

Edit: http://bitwatch.co/uploads/mastercore141212.log the log file

[dexX7]

Amounts as well as SP labels are off for some:

[zathras-crypto]

Great comments thanks guys. In and out today (Saturday's always come with chores!) and will start looking at some of these bits.

A few more details regarding the freeze: I created 15 properties (SpamTokenN) via sendraw_tx....

I am very confused by this one; sendrawtx is a RPC call and leverages no UI elements - the UI can't cause conflicted transactions (it's not involved in the creation of those txs in any way). Could you run the same 15 prop creation test with the daemon only?

Any chance to do a search in the "Lookup Property" tab based on partial information?

Already possible just type say 'mas' for example to get all properties that have the characters 'mas' in the name. No need to use a wildcard. Basically the modus operandi is as follows: 1) Check if the entry is a pure number, if so see if we can find a property under that ID 2) Check if the entry is a base58 encoded address, if so provide user with all properties from that issuer 3) If neither 1 or 2, take text and iterate through all properties comparing if the text is part of the string name of each property

Freeze at startup...

Yep this is intermittent but unresolved - still looking for the cause...

Freeze during run...

Hadn't seen that before? Hmm, CPU usage by QT at the time?

The rest seem to be cosmetic and functional changes that are all straight forward enough. So I really see three major issues: 1) Intermittent failure to display UI at startup 2) Intermittent freeze during running 3) Conflicted transactions

1 is definitely related to UI, 2 is also (assuming daemon doesn't also freeze after leaving it to run), 3 is definitely not UI (UI not involved in TX creation).

Thanks guys :)

[dexX7]

I am very confused by this one; sendrawtx is a RPC call ...

Oh yeah, sure. What I did wasn't really relevant. I used the debug interface, but that likely doesn't matter. The point was: I created the properties, left for some time, came back and saw the freeze. There is no direct connection between those properties and the freeze itself, but because a few of them confirmed and some others not, it narrows down when the freeze happend. The conflicting transactions are probably the result of the freeze while the transactions were in an unconfirmed state.

No need to use a wildcard.

I want to lookup all properties that exist. That's what I was referring to. :)

Hmm, CPU usage by QT at the time?

Didn't check. Unfortunally I'm not very available this weekend, but the bruteforce approach to identify the runtime freeze could be to fire random transactions over and over again until it breaks. The runtime freeze surpassed one confirmed block and froze when accepting the next one as it seems. As mentioned, there wasn't much in the log. Going through the code line by line to find the leak is - at least for me - a to large chuck at once at the moment.

[zathras-crypto]

Thanks mate - I'm still not convinced on the input handling - really need to break into the specifics on what causes inputs to be reused in a subsequent transaction after they've been marked as spent in coin control - I saw a bunch of conflicts together a while back (0.0.7 days) but then none recently. Would love to drill down into exactly what triggers the reuse of inputs (attempted double spend, hence conflicted tx).

On Fri, Dec 12, 2014 at 3:59 PM, dexX7 notifications@github.com wrote:

I am very confused by this one; sendrawtx is a RPC call ...

Oh yeah, sure. What I did wasn't really relevant. I used the debug interface, but that likely doesn't matter. The point was: I created the properties, left for some time, came back and saw the freeze. There is no direct connection between those properties and the freeze itself, but because a few of them confirmed and some others not, it narrows down when the freeze happend. The conflicting transactions are probably the result of the freeze while the transactions were in an unconfirmed state.

— Reply to this email directly or view it on GitHub https://github.com/mastercoin-MSC/mastercore/pull/243#issuecomment-66853773 .

[zathras-crypto]

@zathras-crypto: I can confirm: I merged this into current master, but the UI isn't showing up after startup on Ubuntu 14.04 x64. Clicking on the icon in the task bar is without effect, even a right click isn't possible. Edit: Shutdown via mastercore-cli stop also freezes and does not finish. Edit 2: it starts on second startup.

Can I just clarify the behavior you see? 1) You start a QT build 2) The icon is displayed, but the main UI is not displayed 3) RPC is init and can be queried 4) Using RPC to 'stop' causes a freeze Is that accurate?

Thanks bud Z

[zathras-crypto]

Note to self, tested DEx purchases for tx history in UI - fail, show as receives with 0 amount.

[zathras-crypto]

Amounts as well as SP labels are off for some: {screenie}

Confirmed (the SP creates anyway, can you elaborate on the wrong amount issue?), working on fix

EDIT: Should be fixed now - valid creates will show the correct ID, invalid creates don't generate an ID so that field will be set to N/A.

[dexX7]

Would love to drill down into exactly what triggers the reuse of inputs ..

My first thought was reuse of inputs as well, but I almost can't believe it. Is this truely the definition of "conflicting" transactions or might it just be "some junk that is left over" and classified as conflicting due to the lack of an alternative? Transactions are stored or referenced in the wallet.dat as well as in the transaction databases. My guess: the wallet transactions exist, the database transactions not. But I have no knowledge about the underlying mechanism and structures. (talking about Bitcoin Core layer, not Master Core DBs)

Can I just clarify the behavior you see? 1) You start a QT build 2) The icon is displayed, but the main UI is not displayed 3) RPC is init and can be queried 4) Using RPC to 'stop' causes a freeze Is that accurate?

What I can tell for sure: 1) I started the QT build 2) The icon is displayed 3) No splash screen (IIRC!) 4) No icon interaction 5) "mastercore-cli stop" had no effect

I did not test other RPC commands.

During the runtime freeze: 1) The UI was grey 2) The UI was non responsive and moving, resizing or minimizing was also impossible 3) Icon interaction was possible (menu pops up) 4) Icon interaction had no effect (menu open, but clicks without response) 5) RPC commands resulted in a "pending" state, as if it were waiting for a response

[zathras-crypto]

Brilliant, thanks for the info mate - I'm adding extra debugging in now to try and narrow it down :)

My first thought was reuse of inputs as well, but I almost can't believe it. Is this truely the definition of "conflicting" transactions or might it just be "some junk that is left over"?

See here https://github.com/bitcoin/bitcoin/pull/3669 - also if you grep debug.log for the txid of the conflicted transactions, I'd bet they were rejected by peers for trying to double spend inputs. Another useful tool is to knock out a quick bash script to dump the inputs (note decoderawtransaction/getrawtransaction 1) for that batch of transactions and see if any were duplicated (reused)

[dexX7]

if you grep debug.log for the txid of the conflicted transactions

If I recall correctly, then there was nothing unusual in the logs. Unfortunally I have no access to my "main station" until sunday, so it's all based on memory for now. A very small part of the debug.log is shown in the picture here. The full mastercore.log is here: http://bitwatch.co/uploads/mastercore141212.log

Another useful tool is to knock out a quick bash script to dump the inputs for that batch of transactions and see if any were duplicated (reused)

This is in particular impossible, because there is literally no information about those transactions available. They don't exist, so to speak. I assume they made it into the mempool, but were not stored and then, due to the freeze and fresh startup, removed. (mempool is in memory only as the name suggests.. :)

bitcoin#3669

What I take from it:

"conflicted" transaction-- a transaction created by the wallet that is not in either the blockchain or the memory pool

As I read it: double spents may result in conflicting transactions, but not every conflicting transaction is a double spent.

[zathras-crypto]

This is in particular impossible, because there is literally no information about those transactions available. They don't exist, so to speak.

To show a conflicted transaction in the first place, the transaction has to exist in the wallet file (albeit not in mempool or blockchain) otherwise nothing to conflict - for example on my test instances I can grab the txid of the conflicted tx, push it into gettransaction to get the hex, push the hex into decoderawtransaction to see the inputs. tl:dr; for conflicted transactions whilst no-one else can see them should still be in your wallet :)

[zathras-crypto]

P.S. no worries at all about responses/access to your main box etc - it's the weekend (or so the better half keeps telling me haha)

[dexX7]

There is a reference or some other piece of information about the transactions in the wallet.dat, yes, but when fetching one of those (via getrawtransaction or whatever) it comes sooner or later to fetching the transaction the actual DB:

    if (!GetTransaction(hash, tx, hashBlock, true))
        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "No information available about transaction");

And this one fails, because there no transaction stored in the DB:

[zathras-crypto]

FYI get_raw_transaction {txid} 1 will throw a "No info for tx" error, gettransaction (no raw), take the hex and push into decoderawtransaction should work :)

[dexX7]

Ah, this is very, very good to know! Thanks, I'll try that then.

[zathras-crypto]

OK have a potential fix in for the hang on startup (and potentially while running, same function is called each block to update). Issue appears to be lock related; getHeight requests a lock so that's now only done once at the beginning of the call instead of each transaction and gave the update function an out if it's unable to get the needed locks.

[dexX7]

Meh really? I was hoping it was a more ... well, less opaque problem. For me it sounds as if you were able to reduce the probability of the occurrence of an error, but it still remains?

[zathras-crypto]

For me it sounds as if you were able to reduce the probability of the occurrence of an error, but it still remains?

Testing methodology was very manual yep, by continually starting and stopping the UI over and over I could reproduce the issue. Added a bunch of debugging info to narrow down. After proposed fix just doing same, continually starting and stopping UI and am no longer able to reproduce (from about 30 start/stops).

[zathras-crypto]

Note, no resurface of UI hang at startup during this mornings work :)

[dexX7]

Awesome! I'll battle test and try to shred it apart tomorrow. But my guess: I won't be able to, your usually very precise, when it comes to those things. ;)

Small side note nevertheless: smaller PRs are much easier to audit and test.

[dexX7]

I run clang static analyzer (on the initial version) and there was not much unexpected that I didn't saw in other builds, but there three lines I couldn't make sense of (@zathras-crypto):

./src/qt/forms/overviewpage.ui: Warning: The name 'line' (Line) is already in use, defaulting to 'line1'.

./src/qt/forms/lookupspdialog.ui: Warning: Z-order assignment: '' is not a valid widget.
./src/qt/forms/lookupspdialog.ui: Warning: Z-order assignment: '' is not a valid widget.

Another potential leak (@m21):

mastercore.cpp:1273:13: warning: Assigned value is garbage or undefined
            int64_t BTC_amount = ExodusValues[0];
            ^~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~

On the first look: The value of ExodusValue[0] is not set and undefined, if not on mainnet and if there is no mpexo... output, but at least one money... output. The check that may need a refinement: mastercore.cpp#L1113-L1119 - or of course as alternative: initialize the whole array with zero at the beginning.

Full log:

./scan-build-3.5 --use-analyzer /usr/bin/clang-3.5 make
In file included from alert.cpp:9:
In file included from ./net.h:18:
In file included from ./util.h:15:
./tinyformat.h:283:28: warning: Dereference of null pointer
    if(canConvertToChar && *(fmtEnd-1) == 'c')
                           ^~~~~~~~~~~
1 warning generated.
init.cpp:1049:22: warning: Value stored to 'pindexRescan' during its initialization is never read
        CBlockIndex *pindexRescan = chainActive.Tip();
                     ^~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from main.cpp:6:
In file included from ./main.h:15:
In file included from ./coins.h:8:
In file included from ./core.h:9:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:283:28: warning: Dereference of null pointer
    if(canConvertToChar && *(fmtEnd-1) == 'c')
                           ^~~~~~~~~~~
1 warning generated.
In file included from net.cpp:10:
In file included from ./net.h:18:
In file included from ./util.h:15:
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
1 warning generated.
rpcmining.cpp:350:9: warning: Forming reference to null pointer
        UpdateTime(*pblock, pindexPrev);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
rpcmining.cpp:522:5: warning: Forming reference to null pointer
    UpdateTime(*pblock, pindexPrev);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
In file included from rpcserver.cpp:8:
In file included from ./base58.h:19:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
1 warning generated.
In file included from core.cpp:6:
In file included from ./core.h:9:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:283:28: warning: Dereference of null pointer
    if(canConvertToChar && *(fmtEnd-1) == 'c')
                           ^~~~~~~~~~~
1 warning generated.
In file included from mastercore.cpp:16:
In file included from ./base58.h:19:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:283:28: warning: Dereference of null pointer
    if(canConvertToChar && *(fmtEnd-1) == 'c')
                           ^~~~~~~~~~~
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
mastercore.cpp:230:13: warning: Value stored to 'ret' is never read
            ret += fprintf(fileout, "%s ", DateTimeStrFormat("%Y-%m-%d %H:%M:%S", GetTime()).c_str());
            ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mastercore.cpp:1273:13: warning: Assigned value is garbage or undefined
            int64_t BTC_amount = ExodusValues[0];
            ^~~~~~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~
4 warnings generated.
In file included from mastercore_tx.cpp:3:
In file included from ./base58.h:19:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:283:28: warning: Dereference of null pointer
    if(canConvertToChar && *(fmtEnd-1) == 'c')
                           ^~~~~~~~~~~
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
2 warnings generated.
mastercore_rpc.cpp:1451:25: warning: Value stored to 'bIsMine' is never read
                        bIsMine = false;
                        ^         ~~~~~
1 warning generated.
In file included from mastercore_dex.cpp:3:
In file included from ./base58.h:19:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
1 warning generated.
In file included from mastercore_sp.cpp:3:
In file included from ./base58.h:19:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
1 warning generated.
netbase.cpp:1146:17: warning: Value stored to 's' during its initialization is never read
    const char *s = buf;
                ^   ~~~
1 warning generated.
util.cpp:296:13: warning: Value stored to 'ret' is never read
            ret += fprintf(fileout, "%s ", DateTimeStrFormat("%Y-%m-%d %H:%M:%S", GetTime()).c_str());
            ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
In file included from db.cpp:8:
In file included from ./addrman.h:11:
In file included from ./util.h:15:
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
1 warning generated.
rpcdump.cpp:224:69: warning: Access to field 'nHeight' results in a dereference of a null pointer (loaded from variable 'pindex')
    LogPrintf("Rescanning last %i blocks\n", chainActive.Height() - pindex->nHeight + 1);
                                                                    ^~~~~~~~~~~~~~~
./util.h:117:39: note: expanded from macro 'LogPrintf'
#define LogPrintf(...) LogPrint(NULL, __VA_ARGS__)
                                      ^
1 warning generated.
In file included from wallet.cpp:6:
In file included from ./wallet.h:8:
In file included from ./core.h:9:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:285:36: warning: Dereference of null pointer
    else if(canConvertToVoidPtr && *(fmtEnd-1) == 'p')
                                   ^~~~~~~~~~~
wallet.cpp:305:9: warning: Forming reference to null pointer
        copyTo->mapValue = copyFrom->mapValue;
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
In file included from walletdb.cpp:8:
In file included from ./base58.h:19:
In file included from ./script.h:10:
In file included from ./util.h:15:
./tinyformat.h:309:1: warning: Dereference of null pointer
TINYFORMAT_DEFINE_FORMATVALUE_CHAR(char)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./tinyformat.h:300:12: note: expanded from macro 'TINYFORMAT_DEFINE_FORMATVALUE_CHAR'
    switch(*(fmtEnd-1))                                               \
           ^~~~~~~~~~~
1 warning generated.
/usr/bin/ar: Erzeugen von libmemenv.a
db/log_reader.cc:41:5: warning: Value stored to 'offset_in_block' is never read
    offset_in_block = 0;
    ^                 ~
db/log_reader.cc:85:13: warning: Value stored to 'in_fragmented_record' is never read
            in_fragmented_record = false;
            ^                      ~~~~~
db/log_reader.cc:103:13: warning: Value stored to 'in_fragmented_record' is never read
            in_fragmented_record = false;
            ^                      ~~~~~
3 warnings generated.
In file included from ./util/arena.h:9:0,
                 from ./db/skiplist.h:30,
                 from ./db/memtable.h:11,
                 from db/memtable.cc:5:
db/memtable.cc: In member function ‘void leveldb::MemTable::Add(leveldb::SequenceNumber, leveldb::ValueType, const leveldb::Slice&, const leveldb::Slice&)’:
db/memtable.cc:104:34: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   assert((p + val_size) - buf == encoded_len);
                                  ^
util/bloom.cc: In member function ‘virtual void leveldb::{anonymous}::BloomFilterPolicy::CreateFilter(const leveldb::Slice*, int, std::string*) const’:
util/bloom.cc:50:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     for (size_t i = 0; i < n; i++) {
                            ^
util/logging.cc: In function ‘bool leveldb::ConsumeDecimalNumber(leveldb::Slice*, uint64_t*)’:
util/logging.cc:67:53: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
           (v == kMaxUint64/10 && delta > kMaxUint64%10)) {
                                                     ^
/usr/bin/ar: Erzeugen von libleveldb.a
bitcoin-cli.cpp:76:9: warning: Value stored to 'ret' during its initialization is never read
    int ret = abs(RPC_MISC_ERROR);
        ^~~   ~~~~~~~~~~~~~~~~~~~
1 warning generated.
./src/qt/forms/overviewpage.ui: Warning: The name 'line' (Line) is already in use, defaulting to 'line1'.
./src/qt/forms/lookupspdialog.ui: Warning: Z-order assignment: '' is not a valid widget.
./src/qt/forms/lookupspdialog.ui: Warning: Z-order assignment: '' is not a valid widget.
In file included from transactionrecord.cpp:7:
In file included from ../../src/base58.h:19:
In file included from ../../src/script.h:10:
In file included from ../../src/util.h:15:
../../src/tinyformat.h:283:28: warning: Dereference of null pointer
    if(canConvertToChar && *(fmtEnd-1) == 'c')
                           ^~~~~~~~~~~
1 warning generated.
paymentrequest.pb.cc:51:24: warning: Called C++ object pointer is null
  Output_descriptor_ = file->message_type(0);
                       ^~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Keep in mind: static analysis can serve as hint, but not everything stated is necessarily a problem in reality, nor is any analyzer perfect.

[zathras-crypto]

Exerpt from Skype conversation:

afaik process is similar to: 1) Grab unspent output (including unconfirmed) 2) Use it as an input 3) Mark it spent in UTXO That's why when you send a transaction (specifically selecting inputs via coin control dialog) and send it, even before it has confirmed you cannot go and reselect those inputs in coin control dialog because they are no longer available (ie core doesn't let you double spend) [6:48:36 AM] zathrasc: creating a tx raw should be no different [6:48:42 AM] zathrasc: anyway, I'll try and feed you some testing data on this [6:48:59 AM] zathrasc: I will script up batches of txs designed to expend unspent outputs and run the same tests across bitcoin core and master core [6:49:16 AM] zathrasc: I expect results of: bitcoin core - no double spends, master core - attempted double spends [6:49:33 AM] zathrasc: if the results are: bitcoin core - double spends, master core - double spends then there is nothing we can do, but that result would surprise me tbh [6:49:40 AM] Michael: well I "believe" I've seen conflicts in the normal BTC-mode (mostly UI & mostly on Testnet I think) when trying to send "too fast"; double-spends are impossible of course; but attempts are recorded as conflicts [6:49:55 AM] zathrasc: yeah sorry talking attempted double spends here sure [6:50:53 AM] zathrasc: let me see what I come up with in results :) [6:52:36 AM] zathrasc: ahhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh [6:52:37 AM] zathrasc: lol [6:52:43 AM] zathrasc: just trying to read up on some background here [6:53:06 AM] zathrasc: comments such as "If you send a transaction with the same sending address as the change address, then send from the output of the first transaction back to the sending address, it triggers the error as well." are interesting because we specifically send change back to the sending address [6:53:08 AM] zathrasc: looking further [6:53:20 AM] zathrasc: so basically [6:54:01 AM] zathrasc: send MP transaction, change goes back to sender. Use that back_to_sender_change_output in the next transaction, and that transaction can become conflicted [6:54:12 AM] zathrasc: trying to see if still a bug [6:54:20 AM] zathrasc: was introduced with bitcoin 0.9 apparently [6:55:41 AM] Michael: yeah, something like that :) we are reusing addresses and chaining ins & outs majorly on MP; so I don't consider unconfirmed conflicts too big of a deal [6:56:03 AM] zathrasc: well, my primary concern is a goxxing [6:56:19 AM] zathrasc: when there are issues, what is the common outcome? workarounds [6:56:59 AM | Edited 6:57:32 AM] zathrasc: what would workaround be for the potential for withdrawals on exchanges to not go through due to a conflict? What Gox did - if don't see withdrawal tx on network within say 4 hours reissue withdrawal on new txid [6:57:18 AM] zathrasc: that was used (along with malleability) to repeatedly withdraw from Gox [6:58:05 AM] zathrasc: just saying that for an end user on QT, sending a tx, that doesn't go through due to a conflict is gonna be a PITA but not end of world [6:58:36 AM] zathrasc: for automated systems sending txs, some not going through due to conflicts is problematic right [6:59:26 AM] zathrasc: I'll see if I can grab the inputs used from DexX for his conflicted transactions to give us some more info, and I'll run up some batches of tests [6:59:42 AM] zathrasc: if it is a Bitcoin bug it would be fantastic to fix it and submit upstream :) [6:59:46 AM] zathrasc: anyway will let you know

[zathras-crypto]

Note, this PR is now on hold, we're going to do a daemon only tag first and then lay the UI on top.

@dexX7 what would you consider the most critical points for a daemon release (aside from alerts, consensus testing, and investigating the conflicting tx issue)

Thanks Z

[dexX7]

I'm able to create conflicting and appearingly conflicting transactions at will and this is not related to Master Core.

1) Appearingly conflicting transactions:

1. Send at least two transactions where transaction n + 1 spents an output of transaction n
2. Shutdown or kill the client

This can be done by creating raw transactions or via the GUI with coin control and selecting the output of transaction n to send transaction n + 1.

After restart there is no information about transaction n available in the memory pool and also not in the transaction database. Even though the wallet knows something about the transactions, it appears as if transaction n + 1 tries to spent an input which can't be found.

Assuming the transactions created in the first step are valid, and there is at least one other node which serves as cache, then it's likely the transactions confirm at some point and appear no longer as "conflicting".

A "conflict" is determined by:

// Return depth of transaction in blockchain:
// -1  : not in blockchain, and not in memory pool (conflicted transaction)
//  0  : in memory pool, waiting to be included in a block
// >=1 : this many blocks deep in the main chain
int GetDepthInMainChain(CBlockIndex* &pindexRet) const;

2) Truely conflicting transactions

1. Send or create and broadcast a chain of transactions, say for example A -> B -> C
2. Shutdown or kill the client
3. Restart the client
4. Broadcast another chain which differs at some point, say for example A -> B -> D

We can get away with double spending B, because at the moment of the spend there is no information of the previous spend available.

In the end only one of both conflicting transactions is mined.

---

It is notable that this does not require a kill or some unexpected crash.

I tested this with sending "change" to the same address, as well as to other addresses. It doesn't make a difference and there is really no relation between sending a remaining amount back to the sender or not.

The issue appears to be based on deterministic input selection. If one sends an amount of n coins and there is a set of unspent outputs, it will always select the same unspent output, if n and the set is equal.

As per default Bitcoin Core is configured to avoid spending unconfirmed outputs. Further this is certainly an edge-case and it should not be assumed the user restarts and sends similar transactions over and over again within a short timespan.

In my opinion this issue is something that could be tackled, once/if transactions are no longer created and maintained by the internal wallet class. When doing so, it would be appealing to pick up and redeem multisig dust during this process.

The data is available and all of those unconfirmed transactions are added during startup back to the memory pool:

AppInit2 -> CWallet::ReacceptWalletTransactions -> CMerkleTx::AcceptToMemoryPool -> AcceptToMemoryPool

It then can fail, because some inputs can't be found, because transactions are not added to the mempool in correct order. >_>"

Relevant code part: main.cpp#L906-L915

There is even a nice return flag *pfMissingInputs, so this could probably be solved with some wallet transaction cache during startup and a sorter. This of course, would only work to resolve appearingly conflicting transactions and no attempted double spends.

[dexX7]

Here is a patch: experimental-mcore-reaccept-orphan-wtx

It's basically a copy of main.cpp#L416-L466 and main.cpp#L3763-L3862 without network stuff.

It works, but feels like a shady workaround.

[dexX7]

@zathras-crypto: what would you consider the most critical points for a daemon release?

227, #234 and especially #245 are low hanging fruits, but still important, because they tip topics that are currently not 100 % in line with the spec, ambigious or undefined. It's not about how to "implement" it or whatsoever, but it requires decisions to be made.

If it's going to be a daemon only relase (I'm sad ... :/ I love the new UI), then I suggest to unify the RPC help texts. There are currently two dominant styles (style A vs. style B).

I personally consider the audit trail as very, very important, because it's going to pave the way for a lot more options (Electrum Mastercoin servers maybe?).

And tests, but I think that's more or less clear. It would also help, if interpretPacket transforms into smaller units such as "get sender", "get reference", "has reference", "which class", "get raw data", etc. (just some examples, not necessarily a good choice in particular).

[zathras-crypto]

DexX - brilliant work mate, I'm going to try and replicate your results and reflect on this a bit more... Quick Q (there will be more haha)

2) Truely conflicting transactions Send or create and broadcast a chain of transactions, say for example A -> B -> C Shutdown or kill the client Restart the client Broadcast another chain which differs at some point, say for example A -> B -> D We can get away with double spending B, because at the moment of the spend there is no information of the previous spend available.

On the second run ("broadcast another chain which differs...") you rebroadcast or recreated transaction A using Core? What I'm getting it is rebroadcast = send same hex we already have, recreate = coin control selected an input 2nd time round that should already be flagged as spent

Thanks :)

[dexX7]

It was meant as "you can either create and broadcast a raw transaction or use the coin control to send". What I didn't explicitly state: it's just about spending unspent outputs, how doesn't really matter. It helps to sweep outputs at the beginning, so you can basically force the client to reuse earlier spent, unconfirmed outputs.

The key is, after restart the transactions will hit the mempool in lexicographical order based on their txid. In this context, the example of "A -> B -> C" was actually bad. You should aim for "X -> Y -> A" and "X -> Y -> Z". Hope that helps? :)

A really quick test: sweep the outputs of "oneAddress" then spam:

sendrawtx_MP "oneAddress" "00000000000000020000000000000001" "anotherAddress"

Restart. Spam more. Restart.. ;)

[zathras-crypto]

Re your patch - quick question, why go to all that trouble instead of using the iteration of the wallet?

Just thinking out loud here, but wallet transactions are stored in the order they are pushed to the wallet (basically a send) thus transactions that use outputs of previous transactions (even unconfirmed) are inherently written to the wallet after the prior transaction. Thus why not simply iterate the wallet in order (like for example we do for say listtransactions_MP where we iterate in reverse so newest transactions are at the end of the list)? Anything confirmed is filtered out of CWallet::ReacceptWalletTransactions() anyway and by loading by order in wallet you should thus load prior txs first.

Maybe replacing BOOST_FOREACH(PAIRTYPE(const uint256, CWalletTx)& item, mapWallet) with an iterator or something, shouldn't be much. I have to run now but I'll nose around later.

Thanks again mate :)

[dexX7]

Yeah that sounds much simpler. I basically copied the available code and I'm not really familiar with the wallet.

Edit: the result is lexicographical ordered without providing a comparator, but CWalletTx have:

int64_t nOrderPos;  // position in ordered transaction list

Using this to compare elements works and transactions can be sorted by their appearance in the wallet. This also works for unconfirmed transactions.

It appears as somewhat weaker than truely "chaining" transactions by their in- and outputs, but it still may satisfy all practical requirements.

The case I'm thinking about is when someone broadcasts transactions out of order:

1) External nodes add them to the orphan cache 2) The own wallet would probably reject out of order transactions due to the missing inputs

So let's think about this. Is there an edge case where this approach isn't sufficient?

Edit 2: The approach would roughly be as following:

1. Define a comparator wtxA.nOrderPos < wtxB.nOrderPos
2. Filter mapWallet with wtx.GetDepthInMainChain() < 1
3. Sort result with comparator of step one
4. Add elements of step three to the mempool

[zathras-crypto]

hate accidentally closing a tab with a comment I didn't click 'comment' on :( </end_digression>

[dexX7]

Oh.. :S Quick summary? ;)

[zathras-crypto]

Hehe, in a nutshell, yep we can definitely trip this up with createraw and sendraw for example, to commit out of order transactions. I guess my main 'feel' for this improvement is for our John Doe regular end users to stop them seeing accidental conflicted transactions - while we can purposefully create them, I think loading in wallet order goes a long way to removing the accidental occurrences.

And yep methodology looks good

[zathras-crypto]

And re the initial patch, I guess my feel tends to be if we can resolve this, let's push upstream, and if pushing upstream more likely to get accepted if it's a simple change :)

[dexX7]

Well, the inital approach should cover all cases, the second one probably the practial cases. Just wondering, if there is some case where it would not work, which is not a super edge case. :)

[zathras-crypto]

Yeah just trying to think this through a little more - even if you created them out of order you still couldn't commit them out of order because if you sendraw a transaction spending non-existent outputs (because you haven't committed the original tx yet) the local node will reject it with an error and it won't be committed to wallet right?

I'm trying to look at external influencing factors like you mentioned with an external nodes orphan cache.

I guess in summary:

The case I'm thinking about is when someone broadcasts transactions out of order:

Whilst that can be done for the bitcoin network sure, I'm not sure it's possible to do actually with bitcoin core to have the wallet accept transactions out of order. If I'm correct in thinking you cannot actually sendraw a tx spending non-existent outputs from bitcoin core (and thus cannot commit an out of order tx to the wallet)

sendrawtransaction Submits raw transaction (serialized, hex-encoded) to local node and network. Returns transaction id, or an error if the transaction is invalid for any reason.

Interesting topic, if there's doubt we could always take another look at your original patch, I was just thinking if we could swap the boost::foreach with an ordered iterator it might be a simple change that stood a chance of getting accepted upstream (I think it would be nice to give something back).

[zathras-crypto]

Quick note looking at source I can't see anywhere createraw/signraw/sendraw commit to the wallet at all.

[dexX7]

It looks like it's not added directly, but:

AcceptToMemoryPool -> g_signals.SyncTransaction(hash, tx, NULL);

ConnectBlock -> g_signals.SyncTransaction(block.GetTxHash(i), block.vtx[i], &block);

The signal is associated with to CWallet::SyncTransaction(...). Didn't check, but I'm pretty sure a transaction sent raw ends somehow in AcceptToMemoryPool, too.

But you have a point:

If I'm correct in thinking you cannot actually sendraw a tx spending non-existent outputs from bitcoin core (and thus cannot commit an out of order tx to the wallet)

[dexX7]

https://dev.visucore.com/bitcoin/doxygen/rpcrawtransaction_8cpp.html#ad5aad44f890060f42efb49acf349511e :)