backport: merge bitcoin#19521, #22047, #21796, #21767, #24133, #24921, #27988, partial #24117, #24138, #27405 (assumeutxo: part 5) by kwvg · Pull Request #5501 · dashpay/dash

kwvg · 2023-07-23T17:49:15Z

Additional information

Depends on backport: merge bitcoin#21391, #19550, #15946, #21230, #21252, #21297, #20605, #21575, #22309, #19762 (assumeutxo backports: part 4) #5492

github-actions · 2023-07-23T20:23:56Z

This pull request has conflicts, please rebase.

github-actions · 2023-07-24T16:41:59Z

This pull request has conflicts, please rebase.

github-actions · 2023-07-28T05:21:39Z

This pull request has conflicts, please rebase.

src/init.cpp

src/rpc/blockchain.cpp

src/bench/duplicate_inputs.cpp

src/rest.cpp

UdjinM6

LGTM, utACK

PastaPastaPasta

Seems fine, one question

src/index/base.cpp

knst

3 changes:

this: knst@1c71b08
this refactoring (optional, can be done as an extra PR): knst@0bfefaf
and document block_subsidy in feature_coinstatsindex.py

test/functional/feature_coinstatsindex.py

src/index/base.cpp

kwvg · 2023-08-01T17:29:15Z

A test failure occured because changes brought on by knst's review set the subsidy value for the genesis block to what was set in chainparams (which is correct behaviour, unlike earlier behaviour, which assumed that the genesis block's reward is defined by GetBlockSubsidy).

Remnants of earlier behaviour were still left over in feature_coinstatsindex.py (see here, where the hardcoded value was taken from running GetBlockSubsidy with zero-value args), which resulted in the test failing. The same has been fixed and changes pushed.

knst

utACK (still waiting CI)

UdjinM6

utACK

PastaPastaPasta

utACK for merging via merge commit

Dash uses the height and difficulty of the previous block to calculate the subsidy for the current block... which in the case of the genesis block is block -1, which doesn't exist. Attempting in reading `pprev` which is will evaluate to a `nullptr`, so for any blocks <=0, we fetch the subsidy expected from block 0 from CChainParams.

It's highly unlikely the test will ever deal with chains with >4500 blocks, so only the subset of the subsidy logic that is needed to validate `gettxoutsetinfo` output has been included

excludes: - bfcd60f

…statsindex excludes: - 691d45f (portions)

…nd FastRandomContext::rand_uniform_delay

courtesy of c84390b from bitcoin#25487

…asure durations includes: - fa1d804

#5524) ## Issue being fixed or feature implemented Unlike bitcoin we are using PREVIOUS block in `GetBlockSubsidy()`. That creates special case for genesis block, because it doesn't have previous block. In this special case instead of calling `GetBlockSubsidy` should be used pre-calculated value. To avoid confusion for new code and simplify implementation, there's introduced a new method `GetBlockSubsidyPrev` that has other interface: it takes pointer `CBlockIndex* prev` in agruments instead pair of height + nbits. These changes are follow-up for #5501 ## What was done? Implemented new method `GetBlockSubsidyPrev()` and used instead of `GetBlockSubsidy` when it is more convenient. ## How Has This Been Tested? Run unit/functional tests. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone --------- Co-authored-by: UdjinM6 <[email protected]>

kwvg mentioned this pull request Jul 23, 2023

backport: merge bitcoin#21391, #19550, #15946, #21230, #21252, #21297, #20605, #21575, #22309, #19762 (assumeutxo backports: part 4) #5492

Merged

kwvg changed the title ~~backports: merge bitcoin#19521, #22047, #21796, #21767, #24133, #24138, #24921, #27988, #24117, #27405 (assumeutxo: part 5)~~ backport: merge bitcoin#19521, #22047, #21796, #21767, #24133, #24138, #24921, #27988, #24117, #27405 (assumeutxo: part 5) Jul 23, 2023

github-actions bot added the needs rebase label Jul 23, 2023

kwvg force-pushed the assumeutxo5 branch from ed46381 to 1e5fe6d Compare July 23, 2023 20:46

github-actions bot added needs rebase and removed needs rebase labels Jul 23, 2023

kwvg force-pushed the assumeutxo5 branch from 1e5fe6d to 808a033 Compare July 24, 2023 16:48

github-actions bot added needs rebase and removed needs rebase labels Jul 24, 2023

kwvg force-pushed the assumeutxo5 branch from 808a033 to 4329520 Compare July 28, 2023 07:11

github-actions bot removed the needs rebase label Jul 28, 2023

kwvg force-pushed the assumeutxo5 branch from 4329520 to f4fe0d2 Compare July 28, 2023 07:55

kwvg changed the title ~~backport: merge bitcoin#19521, #22047, #21796, #21767, #24133, #24138, #24921, #27988, #24117, #27405 (assumeutxo: part 5)~~ backport: merge bitcoin#19521, #22047, #21796, #21767, #24133, #24921, #27988, partial #24117, #24138, #27405 (assumeutxo: part 5) Jul 28, 2023

kwvg marked this pull request as ready for review July 28, 2023 11:11

kwvg requested review from PastaPastaPasta, UdjinM6 and knst July 28, 2023 11:11

UdjinM6 requested changes Jul 30, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/bench/duplicate_inputs.cpp Outdated Show resolved Hide resolved

src/rest.cpp Outdated Show resolved Hide resolved

kwvg force-pushed the assumeutxo5 branch from f4fe0d2 to 5ccb4a3 Compare July 30, 2023 12:40

kwvg requested a review from UdjinM6 July 30, 2023 12:44

UdjinM6 added this to the 20 milestone Jul 30, 2023

UdjinM6 previously approved these changes Jul 30, 2023

View reviewed changes

PastaPastaPasta reviewed Aug 1, 2023

View reviewed changes

src/index/base.cpp Outdated Show resolved Hide resolved

kwvg requested a review from PastaPastaPasta August 1, 2023 06:52

knst requested changes Aug 1, 2023

View reviewed changes

test/functional/feature_coinstatsindex.py Outdated Show resolved Hide resolved

src/index/base.cpp Outdated Show resolved Hide resolved

kwvg dismissed UdjinM6’s stale review via 2202ce7 August 1, 2023 16:24

kwvg force-pushed the assumeutxo5 branch from 5ccb4a3 to 2202ce7 Compare August 1, 2023 16:24

kwvg dismissed stale reviews from PastaPastaPasta and knst via 0c2e3b9 August 1, 2023 17:18

kwvg force-pushed the assumeutxo5 branch from 2202ce7 to 0c2e3b9 Compare August 1, 2023 17:18

kwvg requested review from PastaPastaPasta and knst August 1, 2023 17:23

kwvg force-pushed the assumeutxo5 branch from 0c2e3b9 to abca5ef Compare August 1, 2023 17:51

knst approved these changes Aug 1, 2023

View reviewed changes

UdjinM6 approved these changes Aug 1, 2023

View reviewed changes

PastaPastaPasta approved these changes Aug 2, 2023

View reviewed changes

kwvg added 15 commits August 2, 2023 10:19

merge bitcoin#19521: Coinstats Index

040cd92

fix: implement subset of GetBlockSubsidy for feature_coinstatsindex

31bbd52

It's highly unlikely the test will ever deal with chains with >4500 blocks, so only the subset of the subsidy logic that is needed to validate `gettxoutsetinfo` output has been included

fix: adjust values of gettxoutsetinfo to match Dash's mint rate

28dcd38

merge bitcoin#22047: Coinstatsindex follow-ups

d97dcb2

merge bitcoin#21796: Avoid async shutdown on init error

d577d8b

merge bitcoin#21767: Prune g_chainman usage in auxiliary modules

4e9d9a6

partial bitcoin#24117: make indices robust against init aborts

123cf8d

excludes: - bfcd60f

merge bitcoin#24133: Improve robustness of coinstatsindex at restart

6f270bb

partial bitcoin#24138: Commit MuHash and best block together for coin…

c21f23d

…statsindex excludes: - 691d45f (portions)

merge bitcoin#24921: Add time helpers for std::chrono::steady_clock a…

5b338c3

…nd FastRandomContext::rand_uniform_delay

util: add SteadyClock type alias to std::chrono::steady_clock

1e609cf

courtesy of c84390b from bitcoin#25487

partial bitcoin#27405: Use steady clock instead of system clock to me…

a9e3b9f

…asure durations includes: - fa1d804

merge bitcoin#27988: Use same timeout for all index sync

d7b7bbd

test: migrate TestChainSetup's g_txindex guardrails

cc9dcdd

PastaPastaPasta force-pushed the assumeutxo5 branch from abca5ef to cc9dcdd Compare August 2, 2023 15:19

PastaPastaPasta merged commit 0b8e501 into dashpay:develop Aug 2, 2023

knst mentioned this pull request Aug 3, 2023

refactor: new function GetBlockSubsidyPrev for simplification of usage #5524

Merged

5 tasks

kwvg mentioned this pull request Jul 16, 2024

fix: move g_txindex initialization out of erroneous location and into constructor #6119

Merged

5 tasks

Conversation

kwvg commented Jul 23, 2023

Additional information

Uh oh!

github-actions bot commented Jul 23, 2023

Uh oh!

github-actions bot commented Jul 24, 2023

Uh oh!

github-actions bot commented Jul 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kwvg commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kwvg commented Aug 1, 2023 •

edited

Loading