refactor: cleanup {address,spent}index, remove platform-specific types, split out timestamp index #5577

kwvg · 2023-09-14T10:25:51Z

Motivation

Since #914, the source of the code being imported, BitPay, has discontinued their Bitcoin Core fork and the software that relies on it (source) . The set of changes were added into Dash Core in Aug 2016, the last set of changes introduced to the fork were in Sep 2016 (commit).

Dash maintains Insight, a now discontinued BitPay codebase, on its own by forking it (source) and Dash Core developers have had to make modifications to the imported code to make sure it fits within current internal APIs. But aside from that, the code has been more or less, left alone.

It is important to note that a lot has changed from Bitcoin Core 0.12. A dedicated, independent, transaction index was introduced in v0.17 (source), a block filter index was introduced in v0.19 (source) and a coin stats index was introduced in v22 (source), all of which, are currently integrated into Dash Core, making the infrastructure to create indexes, readily available.

When working on #5574, a concern was raised about certain changes that were made to fit with new Dash Core backports. This in turn, inspired looking through the codebase and noticing a disparity in maintenance efforts, also noticing the usage of magic values, repetitive code, outdated code conventions, unclear naming conventions, etc.

Additional Information

This pull request explicitly aims to maintain backwards-compatibility (i.e. requiring no reindexing).
The set of changes include
- Using fixed-length data types (e.g. preferring uint32_t over unsigned int). Guidance was taken from cppreference using the LP64 data model.
- Using clearer variable names (some structs used index to refer to the position of the input/output in the vin/vout vectors and other structs used it to refer to the position of the transaction in the vtx vector).
- Using constructor initialization lists (introduced in C++11)
- Using default member initializers (introduced in C++11)
- Using enums for values within a limited range
  - The current codebase uses raw integers to refer to the types of address (P2SH, P2PKH and P2PK) with 2, 1, 1 respectively, with 0 for unknown. This has been replaced with AddressType
- Using appropriate data types for members/arguments, examples include:
  - CMempoolAddressDeltaKey used an int to mark if a transaction is an input or an output by calling it spending. It would be more appropriate to use a bool as there are no gradient states. A further explanation is present in the commit body, including reasons why this approach may be taken.
  - addressType is represented as a 32-bit member but is (de)serialized as an 8-bit value. It would be more appropriate to use uint8_t instead.
  - The usage of size_t for values, that while influenced by the size of a vector, are (de)serialized as 32-bit values. It was replaced with uint32_t.
  - Some constructor arguments used data types that don't match with the members. This has been resolved as well.

github-actions · 2023-09-24T14:52:36Z

This pull request has conflicts, please rebase.

strong enums (enum class) cannot be converted implicitly to another type, requiring you to either use a static_cast or use to_underlying, which is a part of C++23, which this codebase doesn't support. the idea of scoping a weak enum into a namespace is courtesy of https://stackoverflow.com/a/46294875/13845753

knst

@kwvg ~~I will add to review couple more comments later, WIP~~ done

src/addressindex.h

src/spentindex.h

knst · 2023-09-25T08:39:02Z

src/addressindex.h

@@ -1,5 +1,6 @@
 // Copyright (c) 2009-2010 Satoshi Nakamoto
 // Copyright (c) 2009-2015 The Bitcoin Core developers
+// Copyright (c) 2023 The Dash Core developers


seems as this file never has been in bitcoin so far as I see...

Probably need to remove Statoshi and Bitcoin Core from copyright.

I assume the copyrights were added in because they borrowed logic from Nakamoto-era code and beyond. I was surprised to find that their own copyright was missing so it was added in to make the original author of the code much more apparent.

knst · 2023-09-25T08:43:07Z

src/addressindex.h

@@ -41,21 +41,21 @@ struct CMempoolAddressDelta
 struct CMempoolAddressDeltaKey
 {
 public:
-    int32_t m_address_type;
+    uint8_t m_address_type{AddressType::UNKNOWN};


ok, but there's usage of m_address_type in src/rpc/misc.cpp

better to refactor them too

src/timestampindex.h

src/addressindex.h

there are two problems with "int spending;" - the integer, which implies that there are gradient states when there is none, only boolean (vin is spent, vout is UTXO) - the name, "spending" implies the existence of a middle state, there is none a reason why int may have been used is due to needing it in the comparison struct CMempoolAddressDeltaKeyCompare, though, using an int isn't necessary as when used with a comparison operator, a bool is implicitly converted to an int. see, https://en.cppreference.com/w/cpp/language/implicit_conversion (Integral promotion)

m_address_type is (de)serialized using ser_writedata8, making the maximum numbers of bits read or written to, 8. AddressType does not have values below 0, therefore the data type is changed to better reflect the way it is stored.

the size of size_t is platform-dependent, (de)serialization is done assuming the the value is 32 bits. changed to uint32_t as index value cannot be less than zero.

Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

…type Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

knst

utACK

UdjinM6

👍 ACK (reindexed on testnet with all indexes on, no performance regression vs develop, rpc results look sane)

PastaPastaPasta

utACK for squash merge

kwvg force-pushed the bitcore_idx branch 3 times, most recently from e0c38be to 459c57d Compare September 15, 2023 00:02

kwvg requested a review from UdjinM6 September 18, 2023 15:48

github-actions bot added the needs rebase label Sep 24, 2023

kwvg force-pushed the bitcore_idx branch from 459c57d to c4415d1 Compare September 24, 2023 20:54

github-actions bot removed the needs rebase label Sep 24, 2023

kwvg force-pushed the bitcore_idx branch from c4415d1 to 13e1cf5 Compare September 24, 2023 20:55

kwvg requested review from knst, UdjinM6 and PastaPastaPasta and removed request for UdjinM6 September 24, 2023 21:56

kwvg marked this pull request as ready for review September 24, 2023 21:56

knst requested changes Sep 25, 2023

View reviewed changes

kwvg added 13 commits September 25, 2023 20:19

refactor: cleanup map comparison struct definitions

5950f7c

refactor: cleanup addressindex structs, use fixed-length integers

5397782

refactor: add separators between members, functions and serialization

98e6185

refactor: cleanup spentindex structs, use fixed-length integers

5c34da0

refactor: use uint8_t for m_address_type

6a5e74e

m_address_type is (de)serialized using ser_writedata8, making the maximum numbers of bits read or written to, 8. AddressType does not have values below 0, therefore the data type is changed to better reflect the way it is stored.

refactor: remove size_t usage in spentindex structs

fc2d193

the size of size_t is platform-dependent, (de)serialization is done assuming the the value is 32 bits. changed to uint32_t as index value cannot be less than zero.

docs: add BitPay copyright to file headers

ce3b5ac

refactor: move timestamp index out of spentindex

cc2efac

refactor: move CAddressIndex* out of spentindex

0cc321c

refactor: use static_cast for (de)serializing bool

51203a8

refactor: deduplicate scriptPubKey trimming, index operations

b29ef2e

refactor: remove more code duplication from (dis)connect operations

f47021a

kwvg force-pushed the bitcore_idx branch from 13e1cf5 to 882c3f3 Compare September 25, 2023 17:06

kwvg force-pushed the bitcore_idx branch from 882c3f3 to 6cc930e Compare September 25, 2023 17:23

kwvg and others added 5 commits September 25, 2023 22:57

refactor: add missing headers to {address,timestamp}index

b08a634

refactor: use range-based loops instead of iterators

109f57c

Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

refactor: use structure binding in for loop in rpc/misc

c65a10f

Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

refactor: make AddressType a strong enum, remove uint8_t for address_…

f011c31

…type Co-authored-by: Konstantin Akimov <knstqq@gmail.com>

refactor: consolidate P2PK{H} types to P2PK_OR_P2PKH

93ddd3f

kwvg force-pushed the bitcore_idx branch from 6cc930e to 93ddd3f Compare September 25, 2023 17:28

knst approved these changes Sep 25, 2023

View reviewed changes

UdjinM6 added this to the 20 milestone Sep 26, 2023

UdjinM6 approved these changes Sep 26, 2023

View reviewed changes

PastaPastaPasta approved these changes Sep 27, 2023

View reviewed changes

PastaPastaPasta merged commit 2958aac into dashpay:develop Sep 27, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: cleanup {address,spent}index, remove platform-specific types, split out timestamp index #5577

refactor: cleanup {address,spent}index, remove platform-specific types, split out timestamp index #5577

kwvg commented Sep 14, 2023 •

edited

Loading

github-actions bot commented Sep 24, 2023

knst left a comment •

edited

Loading

knst Sep 25, 2023

kwvg Sep 25, 2023

knst Sep 25, 2023

knst left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

refactor: cleanup {address,spent}index, remove platform-specific types, split out timestamp index #5577

refactor: cleanup {address,spent}index, remove platform-specific types, split out timestamp index #5577

Conversation

kwvg commented Sep 14, 2023 • edited Loading

Motivation

Additional Information

github-actions bot commented Sep 24, 2023

knst left a comment • edited Loading

Choose a reason for hiding this comment

knst Sep 25, 2023

Choose a reason for hiding this comment

kwvg Sep 25, 2023

Choose a reason for hiding this comment

knst Sep 25, 2023

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

kwvg commented Sep 14, 2023 •

edited

Loading

knst left a comment •

edited

Loading