Refactor Events

[jmjatlanta]

Note: This is a re-do of pr #440, but based on the dev branch.

I would like to use this PR to open a dialog into integrating heavier use of C++ features into the codebase. Comments are encouraged. But please send any comments about what I should or should not be working on directly to me instead of commenting here.

Status as of 2021-08-10: I believe the code sufficiently demonstrates concepts that could be used in further refactoring. I believe the code to be bug-free. , but not complete, as both old and new ideologies coexist for discussion and testing purposes. More test cases are needed for edge cases that I have as yet not learned about.

Points for discussion:

1. Implementations in header files: Much of the code I have seen contains little implementation in .cpp files and most in .h header files. The disadvantages of this approach are (1) increased compile times and (2) delicate dependencies in linking libraries or executables. Moving the implementations to .cpp files does have disadvantages as well, such as objects compiled with different parameters can cause different (sometimes hard to diagnose) errors, but it is my opinion that the benefits outweigh the dangers.
2. Use the standard library where it makes sense to avoid platform-specific code. Two changes highlight the benefits here. Mutexes (std::mutex insted of pthread's version), and lists (std::list vs ptr-to-ptr lists).
3. Remove use of pointers where possible (i.e. pass references if possible, const references are even better), use reference counted pointers instead of raw pointers. The std::list vs ptr-to-ptr is an example here as well.
4. Declare necessary variables when you need them, not at the beginning of the function. This often makes the code easier to read.
5. Use exceptions where it makes sense. If the code is littered with if statements for error checking, consider using try/catch. This can reduce the code you need to write, as well as provide a performance boost in some instances if done correctly.
6. Use namespaces. Instead of starting each method name with komdo_, consider using namespace komodo. This reduces name collisions within the codebase (we know what an "event" is in komdo, and don't have to worry about puting "komodo_" in front of it, or a UI event having a name collision with it). This also often makes using your code as a library less cumbersome.
7. Use bool if you only need true/false instead of other integrals. This makes your intent clear.
8. Comment things that may be unclear. I personally use doxygen style comments on top of my functions. I hope everyone can understand my code by reading it, but sometimes it is very inconvenient to do so. Of course the evil side of comments is they are often stale or inaccurate. But the intent often remains.
9. Push implementations to where they belong. A "notary event" needs to be serialized. It is probably best that the code to do so reside close to the definition of what a notary event is. Every serialized "event" has a header, so this is probably a good time to use a hierarchy of "notary_event is an event, and knows how to serialize itself". Now reading or writing it to disk only requires code to tell a notary event to serialize/deserialize itself. The implementation code stays centralized and out of the way.
10. Refactor a little at a time. Knowing then what is known now, everyone would love to rewrite pretty much everything we've ever written. Avoid the urge. Bite-sized subsystems can be tackled. Eating the whole elephant will take time. The majority of the komodo_events.h file breaks point 9 above. I'd love to get rid of it, as well as big chunks of komodo.h for the same reason. But that would make testing and comparing the existing implementation with the refactor too much to take on in this round. So they will remain until they are no longer needed.
11. Header files should be independent. Include all the #include files necessary for the .h. This makes chasing down dependencies less of a problem at the expense of a little compile time. Use #pragma once to help with this.

The above points are simply opinions. I am not here to start wars. If the team decides that any (or all) of these are a bad idea, I'll happily work without using them. Explaining why they are a bad idea helps everyone, but is not required.

My apologies in advance to the reviewers. There is a lot of code movement with (relatively speaking) minor changes. Moving implementations from .h to .cpp touched many seemingly unrelated areas. Doing that in a separate PR would have made both reviews easier, but merging more complicated.

Reading suggestions and References:

• Migrating Legacy Systems
• Komodo Contributing Guide (includes recommending established standards such as Google's or Bjarne Stroustrup)
• Bitcoin Contributor Readme
• Bitcoin Developer Notes and Coding Style Guidelines

[jmjatlanta]

Comment of @dimxy moved from #440

do we need both memread and the new mem_read functions?

I do not think so. I left both as I build out tests, and once I verify all cases are covered, we can eliminate the unnecessary functions.

[jmjatlanta]

My offer may be will be bit strange from first look - but, i would like to offer don't touch anything else in this PR. Why? It's simple. Every PR should be "finalized" at certain point and contains only fixes / additions related to it's initial sense. I mean this PR is for refactoring events, but we already removed marmara stuff in it, moved some routines from .h (headers) to .cpp files and did some other things. Let's leave NPOINTS refactoring for the "second stage" / other PR. Bcz if we didn't do it - it could turn out in "infinite refactoring". Like ... what if i will say that storing events in plain binary file komodostate and having indexes komodostate.ind is suboptimal? yum We could use LevelDB or even SQLite3 DB for that, which is perfectly fits in data / indexes conception and probably have additional mechanisms for data guard, in compare, with just binary file with records? Just want to offer stop adding changes in this PR and focus on double checking everything instead. What will you say?

Strange? By no means. "Scope Creep" is a persistent problem, and I agree with your comment. Unfortunately, I committed it before your reminder was read. Consider my wrist slapped. We'll let @dimxy decide if he wants me to put it in another branch.

[dimxy]

No problem, we could put aside NPOINTS and KOMODO_STATES refactoring (but I believe we need to do this one time and add comments as it is not easy to get the idea of those objects but we have to deal with them in code pretty often. Also it seems NPOINTS is never freed)

[dimxy]

#476 (comment)
this looks a bit weird and better to be refactored.
What would happen for example if someone makes a chain with the name as one of cryptocurrencies in the list?

[jmjatlanta]

#476 (comment)
this looks a bit weird and better to be refactored.
What would happen for example if someone makes a chain with the name as one of cryptocurrencies in the list?

Agreed. The link between CURRENCY and komodo_state is pretty weak. I'm thinking the collection of komodo_state should be dynamic. My first reaction is a map keyed on the symbol, but I need to see all the use cases. I believe this and NPOINTS should become github issues.

[dimxy]

#476 (comment)
this looks a bit weird and better to be refactored.
What would happen for example if someone makes a chain with the name as one of cryptocurrencies in the list?

Agreed. The link between CURRENCY and komodo_state is pretty weak. I'm thinking the collection of komodo_state should be dynamic. My first reaction is a map keyed on the symbol, but I need to see all the use cases. I believe this and NPOINTS should become github issues.

Yes a map looks like a great option and improves readability.
Also good to document the idea of the komodo_state and currency list here, seems this is a part of some app in the code (pax). And, there was earlier a suggestion to move some code into 'experimental' branches and clean the production branches, maybe this code part related to currencies may be just sent there

[DeckerSU]

This line:

komodo/src/komodo_bitcoind.h

Line 35 in 56590a4

uint8_t DecodeMaramaraCoinbaseOpRet(const CScript scriptPubKey,CPubKey &pk,int32_t &height,int32_t &unlockht);

Could be removed as well, as we don't have anything Marmara related anymore.

As well as:

src/cc/marmara.cpp

Also don't needed now.

p.s. To clarify what i doing now - as you probably know we have also Komodo-Qt (KomodoOcean) codebase which is almost similar to original komodod, but with GUI (Qt) part and various improvements. As codebases bit different this PR changes couldn't be easily cherry-picked or merged, so, i transfer changes manually "step-by-step", verifying and checking everything once again. From one side this is additional work and "time waste", but from other side - this is an additional level of checking and testing which wouldn't be superflous in our case, so, don't be surprised if i'll mention this PR in Komodo-Qt changes.

[dimxy]

Could be removed as well, as we don't have anything Marmara related anymore.
all marmara remains need to be removed starting with ASSETCHAINS_MARMARA var (marmara now is a dedicated repo)
there is some if-else logic for marmara in staking code in komodo_bitcoind so let's be accurate with that (I did this in the research branch plz check it if any questions)

[DeckerSU]

@jmjatlanta Seems found a serious bug, that lead to daemon crash during connecting a block + writing an event to komodostate file. Already digged a root of issue:

struct event_notarized : public event
{
    event_notarized() : event(komodo_event_type::EVENT_NOTARIZED, 0), notarizedheight(0), MoMdepth(0) {}
    event_notarized(int32_t ht) : event(EVENT_NOTARIZED, ht), notarizedheight(0), MoMdepth(0) {}
    event_notarized(uint8_t* data, long &pos, long data_len, int32_t height, bool includeMoM = false);
    event_notarized(FILE* fp, int32_t ht, bool includeMoM = false);
    uint256 blockhash;
    uint256 desttxid;
    uint256 MoM; 
    int32_t notarizedheight;
    int32_t MoMdepth; 
    char dest[16];
};

Let's look on dest field, it have size of 16, as it was in original. Ok, let's remember it. Now let's look inside komodo_stateupdate:

        else if ( height != 0 )
        {
            if ( sp != 0 )
            {
                std::shared_ptr<komodo::event_notarized> evt = std::make_shared<komodo::event_notarized>(height);
                memcpy(evt->dest, dest, 65); // (!!!)
                evt->blockhash = sp->NOTARIZED_HASH;
                evt->desttxid = sp->NOTARIZED_DESTTXID;
                evt->notarizedheight = sp->NOTARIZED_HEIGHT;
                evt->MoM = sp->MoM;
                evt->MoMdepth = sp->MoMdepth;
                write_event(evt, fp);
                komodo_eventadd_notarized(sp,symbol,height,evt);
            }
        }

What we doing in case of event_notarized? Copying 65 (!) symbols to the field which have only 16 bytes size? And on next step inside write_event and execution of operator << we have an unhandled exception.

#0  0x00007ffff60d6438 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff60d803a in __GI_abort () at abort.c:89
#2  0x00007ffff61187fa in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7ffff6231fd8 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
#3  0x00007ffff612138a in malloc_printerr (ar_ptr=<optimized out>, ptr=<optimized out>, str=0x7ffff6232078 "free(): corrupted unsorted chunks", action=3) at malloc.c:5020
#4  _int_free (av=<optimized out>, p=<optimized out>, have_lock=0) at malloc.c:3874
#5  0x00007ffff612558c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975
#6  0x0000555555a28708 in __gnu_cxx::new_allocator<char>::deallocate (this=0x7fff2fff5e20, __p=<optimized out>) at /usr/include/c++/5/ext/new_allocator.h:110
#7  std::allocator_traits<std::allocator<char> >::deallocate (__a=..., __n=<optimized out>, __p=<optimized out>) at /usr/include/c++/5/bits/alloc_traits.h:517
#8  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_destroy (__size=<optimized out>, this=0x7fff2fff5e20) at /usr/include/c++/5/bits/basic_string.h:185
#9  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_dispose (this=0x7fff2fff5e20) at /usr/include/c++/5/bits/basic_string.h:180
#10 std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string (this=0x7fff2fff5e20, __in_chrg=<optimized out>) at /usr/include/c++/5/bits/basic_string.h:543
#11 std::__cxx11::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >::~basic_stringbuf (this=0x7fff2fff5dd8, __in_chrg=<optimized out>) at /usr/include/c++/5/sstream:65
#12 std::__cxx11::basic_stringstream<char, std::char_traits<char>, std::allocator<char> >::~basic_stringstream (this=0x7fff2fff5dc0, __in_chrg=<optimized out>, __vtt_parm=<optimized out>) at /usr/include/c++/5/sstream:718
#13 write_event (evt=std::shared_ptr (count 2, weak 0) 0x7fff1c127c60, fp=0x55567341cfc0) at komodo.cpp:186
#14 0x0000555555a2a8e5 in komodo_stateupdate (height=2499613, notarypubs=notarypubs@entry=0x0, numnotaries=numnotaries@entry=0 '\000', notaryid=notaryid@entry=0 '\000', txhash=..., voutmask=voutmask@entry=0, numvouts=numvouts@entry=0 '\000', pvals=0x0, numpvals=0 '\000', KMDheight=0, KMDtimestamp=0, opretvalue=0, opretbuf=0x0, opretlen=0, vout=0, MoM=..., MoMdepth=40) at komodo.cpp:302
#15 0x0000555555a2b766 in komodo_voutupdate (fJustCheck=<optimized out>, isratificationp=<optimized out>, notaryid=notaryid@entry=-1, scriptbuf=0x7fff2fff8980 "jH\257\360\345\064\006\rN'\257$\337\025\020\023ݳ\330\376N\322B\357\257\036\340\tFhY2\225\n\020$&", scriptlen=<optimized out>, height=2499613, txhash=..., i=7, j=1, voutmaskp=0x7fff2fff6c10, specialtxp=0x7fff2fff6bf0, notarizedheightp=0x7fff2fff6bf4, value=0, notarized=1, signedmask=716577440, timestamp=1627488174) at komodo.cpp:516
#16 0x0000555555a2c9e8 in komodo_connectblock (fJustCheck=fJustCheck@entry=false, pindex=<optimized out>, block=...) at komodo.cpp:728
#17 0x00005555556a2c99 in ConnectBlock (block=..., state=..., pindex=pindex@entry=0x7fff1c03da50, view=..., fJustCheck=fJustCheck@entry=false, fCheckPOW=fCheckPOW@entry=true) at main.cpp:3906
#18 0x00005555556aaa41 in ConnectTip (pblock=0x7fff2fffdca0, pindexNew=0x7fff1c03da50, state=...) at main.cpp:4296
...

Why earlier (before refactoring) we didn't face with such error? Answer is in old komodo_eventadd_notarized:

struct komodo_event_notarized N;
...
strncpy(N.dest,dest,sizeof(N.dest)-1);

It was strncpy with sizeof(N.dest)-1, but in refactored implementation we trying to put 65 symbols in event_notarized->dest.

I fixed this error in Komodo-Qt (KomodoOcean) codebase the following way:

DeckerSU/KomodoOcean@5f99d88

In this case we can't change dest field definition in event_notarized to char dest[KOMODO_ASSETCHAIN_MAXLEN]; and do write like memcpy(evt->dest, dest, KOMODO_ASSETCHAIN_MAXLEN);, bcz existing komodostate files already have records and assuming size of dest field as 16. That's why i leave structure and dest field untouched.

[jmjatlanta]

Seems found a serious bug, that lead to daemon crash during connecting a block + writing an event to komodostate file. Already digged a root of issue:

Yes, this is serious. Thank you for the forensic analysis. I am attempting to determine why I put a 65 there, and it may be because of thinking about the true size of KOMODO_ASSETCHAIN_MAXLEN, although why wouldn't I have used that instead of hard-coding 65?

In any case, you are correct in saying that for compatibility reasons, that field must remain 16 bytes, and we are limited to reading 15 of them. I will adjust, test, and look around for similar issues with dest.

Update: I now see what I did. I focused on the length of the local dest instead of event_notarized::dest.

[DeckerSU]

btw, about the way how libcjson.a builds, may be the following approach is more clearly:
https://github.com/DeckerSU/KomodoOcean/blob/5f99d88568e0a49ce654135d59cb8e191f5667cd/src/Makefile.am#L77-L82

# libcjson build
LIBCJSON=libcjson.a
libcjson_a_SOURCES = cJSON.c \
  komodo_cJSON.c \
  komodo_cutils.cpp
libcjson_a_CPPFLAGS=-fPIC
EXTRA_LIBRARIES += $(LIBCJSON)

it's just like "open discussion" how it would better.

p.s. other tests of this PR still continues on my side.

[DeckerSU]

Also need to add clean of libcjson.a on make clean, like this:
DeckerSU/KomodoOcean@53ddb01

[DeckerSU]

@jmjatlanta Windows build of your repo github.com/jmjatlanta/komodo, branch jmj_dev_event_3 failed with:

In file included from komodo_jumblr.h:16,
                 from komodo_jumblr.cpp:15:
uthash.h:895:5: error: ‘uint32_t’ does not name a type; did you mean ‘wint_t’?
     uint32_t count;
     ^~~~~~~~
     wint_t
uthash.h:909:5: error: ‘uint32_t’ does not name a type; did you mean ‘wint_t’?
     uint32_t expand_mult;
     ^~~~~~~~
     wint_t
uthash.h:919:5: error: ‘uint32_t’ does not name a type; did you mean ‘wint_t’?
     uint32_t num_buckets, log2_num_buckets;
     ^~~~~~~~
     wint_t
...
In file included from komodo_jumblr.h:20,
                 from komodo_jumblr.cpp:15:
/usr/share/mingw-w64/include/wincrypt.h:20:11: error: ‘LONG’ does not name a type; did you mean ‘LHND’?
   typedef LONG HRESULT;
           ^~~~
           LHND
In file included from komodo_jumblr.h:20,
                 from komodo_jumblr.cpp:15:
/usr/share/mingw-w64/include/wincrypt.h:238:11: error: ‘ULONG_PTR’ does not name a type; did you mean ‘MAXLONG_PTR’?
   typedef ULONG_PTR HCRYPTHASH;
           ^~~~~~~~~
           MAXLONG_PTR
/usr/share/mingw-w64/include/wincrypt.h:239:11: error: ‘ULONG_PTR’ does not name a type; did you mean ‘MAXLONG_PTR’?
   typedef ULONG_PTR HCRYPTKEY;
           ^~~~~~~~~
           MAXLONG_PTR
...
<tons of other similar errors>
...
make: *** [Makefile:3385: libbitcoin_common_a-komodo_jumblr.o] Error 1

I already made a fix for that:
DeckerSU/KomodoOcean@058fa15

Please, push the similar commit into jmj_dev_event_3 to fix the error and to add fix in this PR.

[DeckerSU]

@jmjatlanta Other error with build test-komodo/test_events.cpp for WIndows:

test-komodo/test_events.cpp: In member function ‘virtual void TestEvents::TestEvents_komodo_faststateinit_test_Test::TestBody()’:
test-komodo/test_events.cpp:172:61: error: no match for ‘operator+’ (operand types are ‘const string_type’ {aka ‘const std::__cxx11::basic_string<wchar_t>’} and ‘const char [12]’)
             const std::string temp_filename = temp.native() + "/kstate.tmp";
                                               ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
In file included from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/stl_algobase.h:67,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/char_traits.h:39,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/ios:40,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/ostream:38,
                 from /home/decker/jmj_dev_event_3/depends/x86_64-w64-mingw32/share/../include/gtest/gtest.h:55,
                 from test-komodo/test_events.cpp:1:
/usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/stl_iterator.h:400:5: note: candidate: ‘template<class _Iterator> std::reverse_iterator<_Iterator> std::operator+(typename std::reverse_iterator<_Iterator>::difference_type, const std::reverse_iterator<_Iterator>&)’
     operator+(typename reverse_iterator<_Iterator>::difference_type __n,
     ^~~~~~~~
/usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/stl_iterator.h:400:5: note:   template argument deduction/substitution failed:
test-komodo/test_events.cpp:172:63: note:   mismatched types ‘const std::reverse_iterator<_Iterator>’ and ‘const char [12]’
             const std::string temp_filename = temp.native() + "/kstate.tmp";
                                                               ^~~~~~~~~~~~~
In file included from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/stl_algobase.h:67,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/char_traits.h:39,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/ios:40,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/ostream:38,
                 from /home/decker/jmj_dev_event_3/depends/x86_64-w64-mingw32/share/../include/gtest/gtest.h:55,
                 from test-komodo/test_events.cpp:1:
/usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/stl_iterator.h:1195:5: note: candidate: ‘template<class _Iterator> std::move_iterator<_IteratorL> std::operator+(typename std::move_iterator<_IteratorL>::difference_type, const std::move_iterator<_IteratorL>&)’
     operator+(typename move_iterator<_Iterator>::difference_type __n,
     ^~~~~~~~
/usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/stl_iterator.h:1195:5: note:   template argument deduction/substitution failed:
test-komodo/test_events.cpp:172:63: note:   mismatched types ‘const std::move_iterator<_IteratorL>’ and ‘const char [12]’
             const std::string temp_filename = temp.native() + "/kstate.tmp";
                                                               ^~~~~~~~~~~~~
In file included from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/string:52,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/locale_classes.h:40,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/ios_base.h:41,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/ios:42,
                 from /usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/ostream:38,
                 from /home/decker/jmj_dev_event_3/depends/x86_64-w64-mingw32/share/../include/gtest/gtest.h:55,
                 from test-komodo/test_events.cpp:1:
/usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/basic_string.h:5927:5: note: candidate: ‘template<class _CharT, class _Traits, class _Alloc> std::__cxx11::basic_string<_CharT, _Traits, _Alloc> std::operator+(const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&, const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>&)’
     operator+(const basic_string<_CharT, _Traits, _Alloc>& __lhs,
     ^~~~~~~~
/usr/lib/gcc/x86_64-w64-mingw32/8.3-posix/include/c++/bits/basic_string.h:5927:5: note:   template argument deduction/substitution failed:
test-komodo/test_events.cpp:172:63: note:   mismatched types ‘const std::__cxx11::basic_string<_CharT, _Traits, _Alloc>’ and ‘const char [12]’
             const std::string temp_filename = temp.native() + "/kstate.tmp";
                                                               ^~~~~~~~~~~~~

This i didn't fix bcz Komodo-Qt have disabled tests build under Windows, but good to fix it anyway in komodo repo.

Update: Fixed it. Here is the commit with changes: DeckerSU/KomodoOcean@9d59e52
Or the patch itself: test_events_windows.patch

[jmjatlanta]

@jmjatlanta Other error with build test-komodo/test_events.cpp for WIndows:

Thank you. I currently do not have a working Windows build environment (WIP). I will update this with the status of my testing when (if?) it is done (seems gmp doesn't compile on WSL/Ubuntu 20.04 right now).

[DeckerSU]

@jmjatlanta Other error with build test-komodo/test_events.cpp for WIndows:

Thank you. I currently do not have a working Windows build environment (WIP). I will update this with the status of my testing when (if?) it is done (seems gmp doesn't compile on WSL/Ubuntu 20.04 right now).

I used Debian GNU/Linux 10 (buster) with mingw-w64/stable 6.0.0-3 (gcc version 8.3-posix 20190406 (GCC)) without any issues, except mentioned above. Probably will test build under Ubuntu 20.04 and WSL/Ubuntu 20.04 later, after i'll finished my current tests. Anyway, now we are closer to accept this #476 .

[DeckerSU]

LGTM now.

[ca333]

secure code reviewed