KomodoPlatform / zebra

An ongoing Rust implementation of a Komodo node. 🦓
Apache License 2.0
3 stars 5 forks source link

Fix address balance overflow, reverse testnet address prefixes (like in kmd) #4

Closed dimxy closed 1 year ago

dimxy commented 1 year ago

Fix address balance calculation overflow due to bigger amounts in kmd (removed MAX_MONEY limit for the balance var) reverse transparent address prefixes for testnet (5<->0)

DeckerSU commented 1 year ago

Fully synced zebrad with this PR changes and decided to check balances of Notary Nodes (as they are the most P2PK users in network) between Zebra getaddressbalance RPC and current KMD explorer. For this I wrote the following bash script:

check-nn-balances-zebra.sh

As it turns out, for some nodes balances are not equal. Then, I guessed that balances are not equal only for nodes who have "in-fly" txes, mean, as NNs active doing splits, active notarizing, some of txes could be in mempool (i.e. just issued from node / just broadcasted and could be not affect balance on zebra or explorer yet). Then I made the same script for S3 notaries with guessing that S3 addresses are not active now and all of their txes are "settled down", and it give me almost equal picture for all nodes except few ones: image Like alien_AR, computergenie_NA ... But it turns out that operators of these nodes used the same addresses in S3 and S6, so, they are not equal at present bcz addresses are "very active".

So, parsing p2pk to legacy script changes looks good for me and I want to approve this PR (bcz it's logic and other things looks perfect), but I want somebody else to keep an eye open on this address balance differences. Probably we need some other tests. Like:

  1. Create 1000 random addresses.
  2. Send 500 txes on each with different values in sat. Like 0.00000051, 0.00000052, etc. (just more than dust threshold), half using P2PK and half using P2PKH script.
  3. Compare these addresses balances after. If everything is Ok - than it means that zebra holding / checking / accounting balances correctly for P2PK and P2PKH both.

Do you like such idea?

dimxy commented 1 year ago

I think address balances basically should not be affected by txns in mempool but indeed, some active address balances may change during tests. Actually in this PR we fixed overflow issues so I think it's good to create tests for big amounts too. I have a testnet config so I could make tests for address balances with many txns and both low and big amounts

DeckerSU commented 1 year ago

I think address balances basically should not be affected by txns in mempool but indeed, some active address balances may change during tests. Actually in this PR we fixed overflow issues so I think it's good to create tests for big amounts too. I have a testnet config so I could make tests for address balances with many txns and both low and big amounts

Anyway, I did some tests with scripting ... script generate random KMD address and send 10 x P2PKH and 10 x P2PKH transactions on it. Example:

[2022-11-14 00:55:29] Pubkey (hex): 02a2599374c6cb42c20d1a36f20aec64567875d8a680247855dc409ea93da56d40 (66)
[2022-11-14 00:55:29] rmd-160 (hex): 8a6c935b6644c5283d7b03b626fa48ea97cb9fec (40)
[2022-11-14 00:55:29] Address: RMu7PxKJbnwnwD5Cu8FxFsTnNe3rJbQPcr
[2022-11-14 00:55:30] p2pk: 7183776f99654532c53b04aae32ded66ecfee3004a1776b9618571aac925808e (0.00010057)
[2022-11-14 00:55:31] p2pkh: 8477d1a6885156319aaab4d3fd3f968bb680888ddc9efd7428f9d6fecd2d13d5 (0.00010057)
[2022-11-14 00:55:32] p2pk: 191878fc735f84ef281d945bf128d2553d49fe54f70e6186e84642fd1eb0a1ea (0.00010058)
[2022-11-14 00:55:33] p2pkh: 427399d64b36381fd1baf144286a4c612a35fb6eb61054e09faa1821b6244ba2 (0.00010058)
[2022-11-14 00:55:34] p2pk: d449c8fa9f54ff1fb7a61791b7425d4f9384bb64a6688495099bd67080c77eb5 (0.00010059)
[2022-11-14 00:55:35] p2pkh: 879c31417171085e3d5cc51460a2ccdedad54c2d13ac36592f2b9ee21d705215 (0.00010059)
[2022-11-14 00:55:36] p2pk: f9f235b1d800dff6490c473cc47b7a930813e34a6712847df682e343d5cba438 (0.00010060)
[2022-11-14 00:55:37] p2pkh: 23b23b6879efcd4f46a07379bccb7a346dfbf4501328a76992f27804470aa6de (0.00010060)
[2022-11-14 00:55:38] p2pk: e1b233570f4c6abdfa887235a9eea540910fd7699fb9b118d1370e12f13b966f (0.00010061)
[2022-11-14 00:55:39] p2pkh: 620e03ebdfda9517ddd5a26083c52138b2b277dfd890e84a963f086547864bb4 (0.00010061)
[2022-11-14 00:55:40] p2pk: bf56309cc27b61324024625fabdf994a1c39df03379d21dd0b63dbacc4567644 (0.00010062)
[2022-11-14 00:55:42] p2pkh: a2da980c314091c8cfe7bd002f4af0b6a6974d7ed35a595ba700235d1d845c95 (0.00010062)
[2022-11-14 00:55:43] p2pk: 17c8e4f6a81abc652cfff35960faa4b4287eeb60d25b803ef7a85f2e89ccf270 (0.00010063)
[2022-11-14 00:55:44] p2pkh: c58096e1f5b76f9d72661f287daf0d58993700c8bd6a98b0aec5795e47a8a560 (0.00010063)
[2022-11-14 00:55:45] p2pk: dc29034e9e190dd57997cf55722aed9dfb19380e61b807983ea2842399e2614a (0.00010064)
[2022-11-14 00:55:46] p2pkh: d2e01eb771856c3d852f683bce7dc94d7d963096c49e6d104feb9a57c6a72947 (0.00010064)
[2022-11-14 00:55:47] p2pk: e1c1b8d15023f56ef94e3322c58eefdc63fa64c050029b2428acc3cf013d3245 (0.00010065)
[2022-11-14 00:55:48] p2pkh: 5822a72dd1e49b629f3dc937b26fe82852beef552facf01ca97e5283723bf261 (0.00010065)
[2022-11-14 00:55:49] p2pk: 2f95c6541224e784d28f93a15fb996cf09d1a0cd632d208532a3ccf1735ced59 (0.00010066)
[2022-11-14 00:55:50] p2pkh: 874632e1c6b3b3311068b057047e3ed28ed5f4476ea8e76d226151c00dd72cef (0.00010066)

Both, explorer and zebrad returns 0.0020123 KMD balance for such addresses. So, I guess all is Ok.

DeckerSU commented 1 year ago

Also test for address RDhEGYScNQYetCyG75Kf8Fg61UWPdwc1C5 which have a transaction(s) operates with big amounts inside (details are here) is passed. zebrad and explorer both return 829854 sat. balance for this address. So, I guess new constraint is work as expected.

dimxy commented 1 year ago

I checked once more - both in zebra and komodo indexes are updated when the txns are in blocks, so mempool txns should not affect the indexes (btw there are additional rpcs to access mempool indexes). I think you may have seen discrepancies in kmd and zebra balances because there is a difference in kmd and zebra p2p protocol implementation, so zebra node may not reach the chain tip due to unanswered 'getblocks' responses https://github.com/dimxy/zebra/issues/15

dimxy commented 1 year ago

closes #6