Make kaspawallet store the utxos sorted by amount #1924

svarogg · 2022-01-04T20:34:26Z

This is a preliminary PR for #1922 .

I'm sending this as a separate PR, since it will immidiately help, even without the compound feature, which might take some time to implement properly.

This changes kaspawallet daemon to always keeps a sorted list of UTXOs.
Thus, on average, less UTXOs would be needed for any created transaction, making it less likely that a too-big transaction would be created.

Note this does incur some performance hit, especially for large wallets, but profiling showed it's probably negligible.

… utxos are spent first - making it less likely a compound will be required

codecov · 2022-01-04T20:49:34Z

Codecov Report

Merging #1924 (799c494) into master (14b2bcb) will decrease coverage by 0.25%.
The diff coverage is 0.00%.

❗ Current head 799c494 differs from pull request most recent head 76a34a2. Consider uploading reports for the commit 76a34a2 to get more accurate results

@@            Coverage Diff             @@
##           master    #1924      +/-   ##
==========================================
- Coverage   61.60%   61.35%   -0.26%     
==========================================
  Files         627      631       +4     
  Lines       29502    29610     +108     
==========================================
- Hits        18176    18166      -10     
- Misses       8751     8865     +114     
- Partials     2575     2579       +4

Impacted Files	Coverage Δ
app/appmessage/error.go	`0.00% <0.00%> (ø)`
app/appmessage/message.go	`71.42% <ø> (ø)`
app/appmessage/rpc_get_balances_by_addresses.go	`0.00% <0.00%> (ø)`
app/rpc/rpc.go	`63.88% <ø> (ø)`
app/rpc/rpchandlers/get_balance_by_address.go	`0.00% <0.00%> (ø)`
app/rpc/rpchandlers/get_balances_by_addresses.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/server/balance.go	`0.00% <0.00%> (ø)`
...allet/daemon/server/create_unsigned_transaction.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/server/server.go	`0.00% <0.00%> (ø)`
cmd/kaspawallet/daemon/server/sync.go	`0.00% <0.00%> (ø)`
... and 6 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 14b2bcb...76a34a2. Read the comment docs.

michaelsutton · 2022-01-05T13:04:56Z

Note that this approach also has some negative effect, since also a medium-size send will use the largest UTXO, while it could of merged a few small UTXOs with out passing the mempool/block limit

michaelsutton · 2022-01-05T13:12:31Z

Note that this approach also has some negative effect, since also a medium-size send will use the largest UTXO, while it could of merged a few small UTXOs with out passing the mempool/block limit

As a simple compromise, perhaps we should first start by accumulating the smallest UTXOs, and only if passing the limit, go to the largest UTXOs. Although I'm sure that with a bit more thinking we can come up with some better greedy approximation algo.

svarogg · 2022-01-05T14:19:36Z

Note that this approach also has some negative effect, since also a medium-size send will use the largest UTXO, while it could of merged a few small UTXOs with out passing the mempool/block limit

I'll push back, saying that it is in the interest of the sender to use as little UTXOs as possible, since:
A. Smaller transaction means less mass (== fees).
B. The less UTXOs he uses, the less addresses does he have to connect.

Thus I'm not convinced what you presented as a downside is indeed a downside (it might be a downside for the network, I'd say a minor one, but not for the user).

svarogg · 2022-01-05T14:22:49Z

Anyway, the main thing this PR does is make sure the utxos are sorted, from which end do we pick them can be discussed and decided upon later down the road.

michaelsutton · 2022-01-05T15:18:14Z

cmd/kaspawallet/daemon/server/sync.go

+func (s *server) insertUTXO(utxo *walletUTXO) {
+	s.utxosSortedByAmount = append(s.utxosSortedByAmount, utxo)
+	// bubble up the new UTXO to keep the UTXOs sorted by value
+	index := len(s.utxosSortedByAmount) - 1


This can be a binary search

e.g. same logic as here https://github.com/kaspanet/kaspad/blob/master/domain/consensus/processes/reachabilitymanager/ordered_tree_node_set.go#L42

You can also use sort.Search, like in findTransactionIndex

nit: I think it's more readable to use code such as func (tobf *TransactionsOrderedByFeeRate) Push, and maybe even more efficient

michaelsutton · 2022-01-05T15:42:43Z

cmd/kaspawallet/daemon/server/sync.go

@@ -244,13 +256,14 @@ func (s *server) refreshExistingUTXOs() error {
 		return err
 	}

-	s.utxos = make(map[externalapi.DomainOutpoint]*walletUTXO, len(getUTXOsByAddressesResponse.Entries))
+	s.utxosSortedByAmount = make([]*walletUTXO, 0, len(getUTXOsByAddressesResponse.Entries))


Binary search above is especially important here since it will reduce the overall insertion cost from n^2 to nlogn

someone235 · 2022-01-08T08:25:37Z

cmd/kaspawallet/daemon/server/sync.go

+func (s *server) insertUTXO(utxo *walletUTXO) {
+	s.utxosSortedByAmount = append(s.utxosSortedByAmount, utxo)
+	// bubble up the new UTXO to keep the UTXOs sorted by value
+	index := len(s.utxosSortedByAmount) - 1


You can also use sort.Search, like in findTransactionIndex

someone235 · 2022-01-08T08:33:13Z

cmd/kaspawallet/daemon/server/sync.go

+func (s *server) insertUTXO(utxo *walletUTXO) {
+	s.utxosSortedByAmount = append(s.utxosSortedByAmount, utxo)
+	// bubble up the new UTXO to keep the UTXOs sorted by value
+	index := len(s.utxosSortedByAmount) - 1


nit: I think it's more readable to use code such as func (tobf *TransactionsOrderedByFeeRate) Push, and maybe even more efficient

(replacing collectUTXOs methods)

michaelsutton · 2022-01-11T10:02:02Z

app/appmessage/rpc_get_balance_by_address.go

@@ -9,7 +9,7 @@ type GetBalanceByAddressRequestMessage struct {

 // Command returns the protocol command string for the message
 func (msg *GetBalanceByAddressRequestMessage) Command() MessageCommand {
-	return CmdGetBalanceByAddressRequestMessage
+	return CmdGetBalancesByAddressesRequestMessage


was this file suppose to change?

michaelsutton · 2022-01-11T10:02:46Z

app/appmessage/rpc_get_balance_by_address.go

@@ -30,7 +30,7 @@ type GetBalanceByAddressResponseMessage struct {

 // Command returns the protocol command string for the message
 func (msg *GetBalanceByAddressResponseMessage) Command() MessageCommand {
-	return CmdGetBalanceByAddressResponseMessage
+	return CmdGetBalancesByAddressesResponseMessage


same here, seems like a mistake and it should remain CmdGetBalanceByAddressResponseMessage

* Report progress percentage when downloading headers in IBD. * Extract reporting logic to a separate type. * Report progress for IBD missing block bodies.

…aspanet#1937) * Split ApplyPruningPointProof to multiple small database transactions. * Increase the timeout duration in TestIBDWithPruning. * Increase the timeout duration in simple-sync. * Explain that if ApplyPruningPointProof fails, the database must be discarded.

) * Increase timeout for pruning proof and add some logs * Show resolving virtual progress as whole percents

) Co-authored-by: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>

* Drop support for p2p v3 * Remove redundant aliases * Remove redundant condition

…aspanet#1945) * Use a time format without ":" to support all file systems * go fmt

Co-authored-by: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>

…mount

someone235 · 2022-02-18T10:40:43Z

cmd/kaspawallet/daemon/server/sync.go

 	}

-	getUTXOsByAddressesResponse, err := s.rpcClient.GetUTXOsByAddresses(addressSet.strings())
+	sort.Slice(utxos, func(i, j int) bool { return utxos[i].UTXOEntry.Amount() > utxos[j].UTXOEntry.Amount() })


Maybe it's better to test it on testnet first with a very big wallet?

n log n should be fine any way

Make kaspawallet store the utxos sorted by amount, so that the bigger…

8370138

… utxos are spent first - making it less likely a compound will be required

svarogg force-pushed the kaspawallet-order-by-amount branch from 49b32ef to 8370138 Compare January 4, 2022 20:35

michaelsutton reviewed Jan 5, 2022

View reviewed changes

someone235 requested changes Jan 8, 2022

View reviewed changes

Start refactor addEntryToUTXOSet

d009585

svarogg force-pushed the kaspawallet-order-by-amount branch from d1ead01 to 5a0b056 Compare January 9, 2022 08:12

Add GetUTXOsByBalances command to rpc

c1b13c4

svarogg force-pushed the kaspawallet-order-by-amount branch from 5a0b056 to 4926054 Compare January 9, 2022 08:58

Store list of addresses, updated with the collectAddresses methods

c1fdc7c

(replacing collectUTXOs methods)

svarogg force-pushed the kaspawallet-order-by-amount branch from 4926054 to c1fdc7c Compare January 9, 2022 09:08

michaelsutton reviewed Jan 11, 2022

View reviewed changes

svarogg and others added 11 commits January 11, 2022 15:34

Fix wrong commands in GetBalanceByAddress

b019bb6

Rename: refreshExistingUTXOs -> refreshUTXOs

799c494

Display progress of IBD process in Kaspad logs (kaspanet#1938)

b1229f7

* Report progress percentage when downloading headers in IBD. * Extract reporting logic to a separate type. * Report progress for IBD missing block bodies.

Increase headers proof timeout and add more progress logs (kaspanet#1939

1cd712a

) * Increase timeout for pruning proof and add some logs * Show resolving virtual progress as whole percents

Add AllowConnectionToDifferentVersions flag to kaspactl (kaspanet#1940)

c85b5d7

Remove duplicate median time calculation on tx validation (kaspanet#1943

69d90fe

) Co-authored-by: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>

Drop support for p2p v3 (kaspanet#1942)

82f0a4d

* Drop support for p2p v3 * Remove redundant aliases * Remove redundant condition

In profile dump file - use a time format supporting all file systems (k…

f58aeb4

…aspanet#1945) * Use a time format without ":" to support all file systems * go fmt

Bump version to v0.11.12 (kaspanet#1941)

13a09da

Co-authored-by: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>

Merge remote-tracking branch 'origin/dev' into kaspawallet-order-by-a…

76a34a2

…mount

someone235 approved these changes Feb 18, 2022

View reviewed changes

someone235 merged commit 16336b0 into kaspanet:master Feb 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make kaspawallet store the utxos sorted by amount #1924

Make kaspawallet store the utxos sorted by amount #1924

svarogg commented Jan 4, 2022

codecov bot commented Jan 4, 2022 •

edited

Loading

michaelsutton commented Jan 5, 2022

michaelsutton commented Jan 5, 2022

svarogg commented Jan 5, 2022

svarogg commented Jan 5, 2022

michaelsutton Jan 5, 2022

michaelsutton Jan 5, 2022

someone235 Jan 8, 2022

someone235 Jan 8, 2022

michaelsutton Jan 5, 2022

someone235 Jan 8, 2022

someone235 Jan 8, 2022

michaelsutton Jan 11, 2022

michaelsutton Jan 11, 2022

someone235 Feb 18, 2022

michaelsutton Feb 18, 2022

Make kaspawallet store the utxos sorted by amount #1924

Make kaspawallet store the utxos sorted by amount #1924

Conversation

svarogg commented Jan 4, 2022

codecov bot commented Jan 4, 2022 • edited Loading

Codecov Report

michaelsutton commented Jan 5, 2022

michaelsutton commented Jan 5, 2022

svarogg commented Jan 5, 2022

svarogg commented Jan 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2022 •

edited

Loading