Implement UTXO set data structure #194
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
AndrejMitrovic
added
the
type-feature
AndrejMitrovic
force-pushed
the
utxo-set
branch
3 times, most recently
from
a2962fa
to
9ebf531
Compare
AndrejMitrovic
changed the title
|
First style nitpick: |
AndrejMitrovic
force-pushed
the
utxo-set
branch
2 times, most recently
from
01b2407
to
422cbc8
Compare
|
Looks like there are issues with connections in our system integrations test:
|
|
This is missing some tests, and needs a rebase. |
AndrejMitrovic
force-pushed
the
utxo-set
branch
2 times, most recently
from
7756adf
to
255f6d7
Compare
|
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
force-pushed
the
utxo-set
branch
from
255f6d7
to
21b65c6
Compare
AndrejMitrovic
added
the
status-blocked
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
21b65c6
to
9566cbb
Compare
AndrejMitrovic
removed
the
status-blocked
AndrejMitrovic
force-pushed
the
utxo-set
branch
2 times, most recently
from
6a49c88
to
4125601
Compare
|
Ok this is unblocked and ready for review. |
|
There is one issue with the current implementation. The block creation is triggered by an incoming Transaction (in 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
force-pushed
the
utxo-set
branch
2 times, most recently
from
67a59ac
to
90aef76
Compare
|
Erg sorry, needs another rebase |
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
90aef76
to
1d535ec
Compare
AndrejMitrovic
force-pushed
the
utxo-set
branch
3 times, most recently
from
3d3b04f
to
4acbc65
Compare
Previously the count parameter was ignored.
The UTXOFinder delegate should not return pointers, as the memory it refers to may point to a temporary.
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
4acbc65
to
2c73973
Compare
| @@ -0,0 +1,292 @@ | |||
| /******************************************************************************* | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
You can just use ulong(...) at call site though, but I don't mind if we have a function for it.
There was a problem hiding this comment.
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 |
| } | ||
| catch (Exception) | ||
| { | ||
| assert(0); |
There was a problem hiding this comment.
scope (failure) assert(0); at the beginning of the function
source/agora/test/Ledger.d
Outdated
| node_1.putTransaction(backup_tx); | ||
| containSameBlocks(nodes, 3).retryFor(3.seconds); // new block finally created | ||
|
|
||
| setLogLevel(LogLevel.error); |
There was a problem hiding this comment.
source/agora/node/Ledger.d
Outdated
|
|
||
| public bool isValidTransaction (const ref Transaction tx) nothrow @safe | ||
| { | ||
| this.utxo_set.reset(); |
There was a problem hiding this comment.
This is quite dangerous, but okay for now. Maybe open an issue about it ?
There was a problem hiding this comment.
Badly named method, I'll rename it.
source/agora/node/Ledger.d
Outdated
| (tx) | ||
| { | ||
| try | ||
| { |
There was a problem hiding this comment.
Love it. (the feature)
I replaced it everywhere else but here, missed it.
There was a problem hiding this comment.
One of Kenji's many stroke of genius
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
2c73973
to
651125c
Compare
|
Made some changes: https://github.com/bpfkorea/agora/compare/2c739737594bce742651b69bdb1f964d53612688..651125ca221fb8fcd1b7aadc4829fd3640217ea6 The |
|
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. |
|
Let's leave it for a following PR. |
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
651125c
to
59ce7b2
Compare
|
Rebased again. |
The UTXOSet class uses an SQLite database as a backing store
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
59ce7b2
to
d1b2587
Compare
AndrejMitrovic
force-pushed
the
utxo-set
branch
2 times, most recently
from
76676b7
to
5d91f8d
Compare
|
Got a stack trace of where it's stuck waiting forever locally: |
AndrejMitrovic
force-pushed
the
utxo-set
branch
from
5d91f8d
to
db33ab3
Compare
|
Here's another one, slightly different: However both time it's in the |
|
@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. |
|
I'm doing another rebuild. If this passes, I'll merge it so we can unblock everyone else that needs UTXOs. |
|
Green again. |
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.