Skip to content
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

Implement UTXO set data structure #194

Merged
merged 4 commits into from Sep 6, 2019
Merged

Conversation

AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Aug 16, 2019

Things to consider:

- The storage is split in two, the hot cache (hashmap) and the cold storage (SQLite). Moving from the hot cache to the cold storage is done by maintaining an array acting as a queue. But this is very basic, and we could go with a linked-list instead.
- On shutdown, I just save everything into the cold store (the SQLite database). But ideally we should be able to restore the hot items (might not be super-important on node startup, we'll see..).

Edit: I've removed the "hot cache", it was premature optimization. And it actually hid a bug in SQLite indexing.

  • If the UTXO database couldn't be loaded from disk, we should attempt to either retrieve it from other nodes (introduce getUtxoSet() entry-point), or rebuild it from the blockchain data we have (much more costly though).

@codecov
Copy link

@codecov codecov bot commented Aug 16, 2019

Codecov Report

Merging #194 into v0.x.x will decrease coverage by 0.12%.
The diff coverage is 87.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x     #194      +/-   ##
==========================================
- Coverage   81.75%   81.62%   -0.13%     
==========================================
  Files          49       49              
  Lines        2510     2591      +81     
==========================================
+ Hits         2052     2115      +63     
- Misses        458      476      +18
Flag Coverage Δ
#unittests 81.62% <87.32%> (-0.13%) ⬇️
Impacted Files Coverage Δ
source/agora/test/Ledger.d 98.46% <100%> (+0.36%) ⬆️
source/agora/common/TransactionPool.d 96.1% <100%> (+1.81%) ⬆️
source/agora/test/Base.d 89.81% <100%> (+0.09%) ⬆️
source/agora/consensus/Validation.d 93.65% <73.91%> (-0.38%) ⬇️
source/agora/node/Node.d 86.66% <75%> (-2.23%) ⬇️
source/agora/consensus/data/UTXOSet.d 84.78% <84.78%> (ø)
source/agora/node/Ledger.d 94.04% <88.88%> (+0.92%) ⬆️
source/agora/network/NetworkClient.d 92.85% <0%> (-3.58%) ⬇️
source/agora/network/NetworkManager.d 66.42% <0%> (-2.15%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b2e55d...db33ab3. Read the comment docs.

@AndrejMitrovic AndrejMitrovic added the type-feature label Aug 16, 2019
@AndrejMitrovic AndrejMitrovic force-pushed the utxo-set branch 3 times, most recently from a2962fa to 9ebf531 Compare Aug 22, 2019
@AndrejMitrovic AndrejMitrovic marked this pull request as ready for review Aug 22, 2019
@AndrejMitrovic AndrejMitrovic changed the title [wip] [preview] Implement UTXO set data structure Implement UTXO set data structure Aug 22, 2019
@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Aug 22, 2019

First style nitpick: Utxo => UTXO

@AndrejMitrovic AndrejMitrovic force-pushed the utxo-set branch 2 times, most recently from 01b2407 to 422cbc8 Compare Aug 22, 2019
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Aug 22, 2019

Looks like there are issues with connections in our system integrations test:

object.Exception@submodules/vibe-core/source/vibe/core/net.d(229): Failed to connect to 127.0.0.1:4000: refused

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Aug 22, 2019

This is missing some tests, and needs a rebase.

@AndrejMitrovic AndrejMitrovic force-pushed the utxo-set branch 2 times, most recently from 7756adf to 255f6d7 Compare Aug 26, 2019
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Aug 27, 2019

This pull request will have to be split up. While adding tests, I've realized I need to be able to skip over transactions which double-spend from the pool. I have a set of working commits, I'll submit the PRs.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Aug 27, 2019

This PR now requires #274 and #275.

Once those two are merged, I'll rebase this one.

It now includes a double-spend networking test-case.

@AndrejMitrovic AndrejMitrovic added the status-blocked label Aug 27, 2019
@AndrejMitrovic AndrejMitrovic removed the status-blocked label Sep 2, 2019
@AndrejMitrovic AndrejMitrovic force-pushed the utxo-set branch 2 times, most recently from 6a49c88 to 4125601 Compare Sep 2, 2019
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 2, 2019

Ok this is unblocked and ready for review.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 2, 2019

There is one issue with the current implementation. The block creation is triggered by an incoming Transaction (in acceptTransaction()). This means whenever a new transaction comes in, and the pool contains more than 8 transactions, a block will be created.

But this also means blocks will be created at the fastest rate possible, there will never be a case where the transaction pool will contain more than 8 valid non-spending transactions.

This will have to be changed so the block is created at specific intervals (perhaps time intervals). Actually, this depends a lot on the consensus (settlement layer), so I think this should be worked on later when we work on consensus, and not in this PR.

@AndrejMitrovic AndrejMitrovic force-pushed the utxo-set branch 2 times, most recently from 67a59ac to 90aef76 Compare Sep 2, 2019
@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Sep 3, 2019

Erg sorry, needs another rebase

@AndrejMitrovic AndrejMitrovic force-pushed the utxo-set branch 3 times, most recently from 3d3b04f to 4acbc65 Compare Sep 4, 2019
Previously the count parameter was ignored.
The UTXOFinder delegate should not return pointers,
as the memory it refers to may point to a temporary.
Copy link
Contributor

@Geod24 Geod24 left a comment

Nice

@@ -0,0 +1,292 @@
/*******************************************************************************
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message mentions "UtxoSet" (UTXOSet), and the "hot cache" which have been removed.


public void updateUtxoCache (const ref Transaction tx) nothrow @safe
{
foreach (input; tx.inputs)
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref ?

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For optimization? I could do const ref I guess.


Get the combined hash of the previous hash and index.
This makes sure the index is always of the same type,
as mixing different-sized uint/ulong would create different hashes.
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use ulong(...) at call site though, but I don't mind if we have a function for it.

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wrote it like that, but then I realized I had three call sites where I'm casting to ulong, and I want to avoid adding duplicate comments at the call site explaining why it's done this way.

Also, tracking down the hashing "bug" took a good portion of my time.

Find an unspent Output in the UTXO set.

Params:
tx_hash = the hash of transation
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mismatch

}
catch (Exception)
{
assert(0);
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope (failure) assert(0); at the beginning of the function

source/agora/test/Ledger.d Show resolved Hide resolved
node_1.putTransaction(backup_tx);
containSameBlocks(nodes, 3).retryFor(3.seconds); // new block finally created

setLogLevel(LogLevel.error);
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


public bool isValidTransaction (const ref Transaction tx) nothrow @safe
{
this.utxo_set.reset();
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite dangerous, but okay for now. Maybe open an issue about it ?

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Badly named method, I'll rename it.

source/agora/node/Ledger.d Show resolved Hide resolved
(tx)
{
try
{
Copy link
Contributor

@Geod24 Geod24 Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope (failure) ...

Copy link
Contributor

@Geod24 Geod24 Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it. (the feature)

I replaced it everywhere else but here, missed it.

Copy link
Contributor

@Geod24 Geod24 Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of Kenji's many stroke of genius

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 5, 2019

Made some changes: https://github.com/bpfkorea/agora/compare/2c739737594bce742651b69bdb1f964d53612688..651125ca221fb8fcd1b7aadc4829fd3640217ea6

The reset() function was a leaky abstraction, I've removed it. :)

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 5, 2019

There is an issue with this code, it's not re-entrant.

There could be a simple way to do this, I could return a struct with a set. However, that allocates. We could perhaps use a buffer returned from some memory allocator.. Anyway for now we could go with allocations, and work on improving it later.

@Geod24 do you want me to work on this in a separate PR, or as part of this? I don't want to delay further progress on the freezing UTXO, for example.

@Geod24
Copy link
Contributor

@Geod24 Geod24 commented Sep 5, 2019

Let's leave it for a following PR.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 5, 2019

Rebased again.

Geod24
Geod24 approved these changes Sep 5, 2019
The UTXOSet class uses an SQLite database as a
backing store
@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Got a stack trace of where it's stuck waiting forever locally:

* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSEGV
  * frame #0: 0x00007fff64fed86a libsystem_kernel.dylib`__psynch_cvwait + 10
    frame #1: 0x00007fff650ac56e libsystem_pthread.dylib`_pthread_cond_wait + 722
    frame #2: 0x0000000100712c60 agora`_D4core4sync9condition9Condition4waitMFZv + 32
    frame #3: 0x000000010009f94e agora`_D3std11concurrency10MessageBox__T3getTDFNaNbNiNfS6geod249LocalRest8ResponseZvTPFNaNiNfCQDiQDh14LinkTerminatedZvTPFNaNiNfCQEqQEp15OwnerTerminatedZvTPFNfSQFv7variant__T8VariantNVmi32ZQpZvZQFxMFMQFyMQEoMQDkMQCfZb(this=0x0000000103007f20, _param_0=void delegate(geod24.LocalRest.Response _param_0) pure nothrow @nogc @safe @ 0x00007ffeefbff020, _param_1=(agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZ9__lambda2FNaNiNfCQDhQDg14LinkTerminatedZv at concurrency.d:779), _param_2=(agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZ9__lambda3FNaNiNfCQDhQDg15OwnerTerminatedZv at concurrency.d:780), _param_3=(agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZ9__lambda4FNfSQDd7variant__T8VariantNVmi32ZQpZv at concurrency.d:781)) at concurrency.d:2148:29
    frame #4: 0x000000010002577b agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZQBh at concurrency.d:775:5
    frame #5: 0x0000000100026cec agora`_D6geod249LocalRest__T9RemoteAPITC5agora4test4Base7TestAPIZQBl14putTransactionMFNfSQBx9consensus4data11TransactionQnZ9__lambda3MFNeZSQFbQEx8Response at LocalRest.d-mixin-706:723:33
    frame #6: 0x0000000100018ed9 agora`_D6geod249LocalRest__T9RemoteAPITC5agora4test4Base7TestAPIZQBl14putTransactionMFNfSQBx9consensus4data11TransactionQnZv(this=0x000000010e3689f0, _param_0=Transaction @ 0x00007ffeefbff2e0) at LocalRest.d-mixin-706:714:21
    frame #7: 0x000000010003134a agora`_D5agora4test10BanManager17__unittest_L28_C1FZ__T9__lambda5TSQCh9consensus4data11TransactionQnZQBuMFNfQBqZv(tx=Transaction @ 0x00007ffeefbff310) at BanManager.d:79:24
    frame #8: 0x0000000100030def agora`_D3std9algorithm9iteration__T4eachS5agora4test10BanManager17__unittest_L28_C1FZ9__lambda5Z__TQCmTASQCm9consensus4data11TransactionQnZQEaMFNfKQBsZEQFo8typecons__T4FlagVAyaa4_65616368ZQv(r=<unavailable>) at iteration.d:997:29
    frame #9: 0x0000000100030422 agora`_D5agora4test10BanManager17__unittest_L28_C1FZv at BanManager.d:79:5
    frame #10: 0x000000010071224b agora`_D4core7runtime18runModuleUnitTestsUZ14__foreachbody2MFPS6object10ModuleInfoZi + 27
    frame #11: 0x0000000100739f02 agora`_D2rt5minfo17moduleinfos_applyFMDFyPS6object10ModuleInfoZiZ14__foreachbody2MFKSQCz19sections_elf_shared3DSOZi + 50
    frame #12: 0x000000010073aa6a agora`_D2rt19sections_elf_shared3DSO7opApplyFMDFKSQBqQBqQyZiZi + 58
    frame #13: 0x0000000100739eac agora`_D2rt5minfo17moduleinfos_applyFMDFyPS6object10ModuleInfoZiZi + 28
    frame #14: 0x000000010072650f agora`_D6object10ModuleInfo7opApplyFMDFPSQBhQBdZiZi + 31
    frame #15: 0x0000000100712000 agora`runModuleUnitTests + 208
    frame #16: 0x000000010073153f agora`_D2rt6dmain211_d_run_mainUiPPaPUAAaZiZ6runAllMFZv + 31
    frame #17: 0x0000000100731383 agora`_d_run_main + 547
    frame #18: 0x0000000100031485 agora`main(argc=1, argv=0x00007ffeefbff9c8) at __entrypoint.d:8:21
    frame #19: 0x00007fff64eb53d5 libdyld.dylib`start + 1
    frame #20: 0x00007fff64eb53d5 libdyld.dylib`start + 1
(lldb)

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Here's another one, slightly different:

rror: need to add support for DW_TAG_base_type 'char' encoded with DW_ATE = 0x10, bit_size = 8
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSEGV
  * frame #0: 0x00007fff64fed86a libsystem_kernel.dylib`__psynch_cvwait + 10
    frame #1: 0x00007fff650ac56e libsystem_pthread.dylib`_pthread_cond_wait + 722
    frame #2: 0x0000000100712c60 agora`_D4core4sync9condition9Condition4waitMFZv + 32
    frame #3: 0x000000010009f94e agora`_D3std11concurrency10MessageBox__T3getTDFNaNbNiNfS6geod249LocalRest8ResponseZvTPFNaNiNfCQDiQDh14LinkTerminatedZvTPFNaNiNfCQEqQEp15OwnerTerminatedZvTPFNfSQFv7variant__T8VariantNVmi32ZQpZvZQFxMFMQFyMQEoMQDkMQCfZb(this=0x0000000103a07f20, _param_0=void delegate(geod24.LocalRest.Response _param_0) pure nothrow @nogc @safe @ 0x00007ffeefbff010, _param_1=(agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZ9__lambda2FNaNiNfCQDhQDg14LinkTerminatedZv at concurrency.d:779), _param_2=(agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZ9__lambda3FNaNiNfCQDhQDg15OwnerTerminatedZv at concurrency.d:780), _param_3=(agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZ9__lambda4FNfSQDd7variant__T8VariantNVmi32ZQpZv at concurrency.d:781)) at concurrency.d:2148:29
    frame #4: 0x000000010002577b agora`_D3std11concurrency__T11receiveOnlyTS6geod249LocalRest8ResponseZQBqFZQBh at concurrency.d:775:5
    frame #5: 0x0000000100025c1c agora`_D6geod249LocalRest__T9RemoteAPITC5agora4test4Base7TestAPIZQBl8shutdownMFNfZ9__lambda2MFNeZSQDmQDi8Response at LocalRest.d-mixin-706:723:33
    frame #6: 0x0000000100025950 agora`_D6geod249LocalRest__T9RemoteAPITC5agora4test4Base7TestAPIZQBl8shutdownMFNfZv(this=0x000000010dc01180) at LocalRest.d-mixin-706:714:21
    frame #7: 0x000000010002a745 agora`_D5agora4test4Base14TestAPIManager8shutdownMFZ__T9__lambda1TC6geod249LocalRest__T9RemoteAPITCQDnQDkQDi7TestAPIZQBeZQCoFNfQCjZv(a=0x000000010dc01180) at Base.d:174:30
    frame #8: 0x000000010002a767 agora`_D3std9algorithm9iteration__T4eachS5agora4test4Base14TestAPIManager8shutdownMFZ9__lambda1Z__TQCmTHSQCm6common6crypto3Key9PublicKeyC6geod249LocalRest__T9RemoteAPITCQEyQEvQEt7TestAPIZQBeZQGaMFKQDqZ14__foreachbody2MFNfKQDiZi(e=<unavailable>) at iteration.d:997:29
    frame #9: 0x000000010072c163 agora`_aaApply + 83
    frame #10: 0x000000010002091a agora`_D3std9algorithm9iteration__T4eachS5agora4test4Base14TestAPIManager8shutdownMFZ9__lambda1Z__TQCmTHSQCm6common6crypto3Key9PublicKeyC6geod249LocalRest__T9RemoteAPITCQEyQEvQEt7TestAPIZQBeZQGaMFNfKQDsZEQHo8typecons__T4FlagVAyaa4_65616368ZQv(r=<unavailable>) at iteration.d:993:21
    frame #11: 0x0000000100015c88 agora`_D5agora4test4Base14TestAPIManager8shutdownMFZv(this=0x000000010dbfda60) at Base.d:174:9
    frame #12: 0x0000000100030adc agora`_D5agora4test10BanManager17__unittest_L28_C1FZv at BanManager.d:44:17
    frame #13: 0x000000010071224b agora`_D4core7runtime18runModuleUnitTestsUZ14__foreachbody2MFPS6object10ModuleInfoZi + 27
    frame #14: 0x0000000100739f02 agora`_D2rt5minfo17moduleinfos_applyFMDFyPS6object10ModuleInfoZiZ14__foreachbody2MFKSQCz19sections_elf_shared3DSOZi + 50
    frame #15: 0x000000010073aa6a agora`_D2rt19sections_elf_shared3DSO7opApplyFMDFKSQBqQBqQyZiZi + 58
    frame #16: 0x0000000100739eac agora`_D2rt5minfo17moduleinfos_applyFMDFyPS6object10ModuleInfoZiZi + 28
    frame #17: 0x000000010072650f agora`_D6object10ModuleInfo7opApplyFMDFPSQBhQBdZiZi + 31
    frame #18: 0x0000000100712000 agora`runModuleUnitTests + 208
    frame #19: 0x000000010073153f agora`_D2rt6dmain211_d_run_mainUiPPaPUAAaZiZ6runAllMFZv + 31
    frame #20: 0x0000000100731383 agora`_d_run_main + 547
    frame #21: 0x0000000100031485 agora`main(argc=1, argv=0x00007ffeefbff9c8) at __entrypoint.d:8:21
    frame #22: 0x00007fff64eb53d5 libdyld.dylib`start + 1
    frame #23: 0x00007fff64eb53d5 libdyld.dylib`start + 1

However both time it's in the BanManager.d tests. I'll try disabling that test.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

@Geod24 Disabling the BanManager test gets rid of the infinite wait issue.

I think the bug might be somewhere in the handling of filtered requests in LocalRest, because the BanManager makes heavy use of that feature there. I'll submit an issue about re-enabling the BanManager tests later if you merge this PR.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

I'm doing another rebuild. If this passes, I'll merge it so we can unblock everyone else that needs UTXOs.

@AndrejMitrovic
Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic commented Sep 6, 2019

Green again.

@Geod24 Geod24 merged commit 261b054 into bosagora:v0.x.x Sep 6, 2019
3 checks passed
@bpalaggi bpalaggi added this to the 1. Full Node milestone Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants