backport: merge bitcoin#23842, #24714, #21059, #23994, #19866, #19935, #23756, #19064, #23411, #20080 (util backports) #5546

kwvg · 2023-08-24T08:03:51Z

Additional Information

Dependency for refactor: subsume CoinJoin objects under CJContext, deglobalize coinJoin{ClientQueueManager,Server} #5337
Earlier variants of backporting bitcoin#19064 resulted in consistent UnicodeDecodeError terminations when running combine_logs.py (GitLab build, again and again)
- This has been resolved with squashing changes from 208abd4 into the backport (Thanks @UdjinM6!)

knst · 2023-08-24T09:08:27Z

I feel like it is not good idea for rushing with backports such as "merge bitcoin#22976: Rename overloaded int GetArg to GetIntArg"

By rough estimation, there are more than 500 backports to do, and each of them who use old version of GetArg will create an extra conflict that requires a manual resolving.

Of course, each particular fix in each particular case is trivial, the compiler even will show it as a compilation error, so, it won't cause any real issue.

But disadvantage of doing bitcoin#22976 at this moment is an overall slowdown of backporting process. Current backport status of full coverage (98-99%) at this point in v21:

Q_Staged (merged to knstqq local repo) ae32e5c Merge bitcoin#18669: log: Use Join() helper when listing log categories FALSE 8/21/2023 3 3 FALSE

If get list all of commit between them 22976 and 18669:

 git rev-list --ancestry-path ae32e5ce3d..825f4a64e612dab62cd0a73b2afe32dac5e0c69f

and grep 'GetArg' after that:

git rev-list --ancestry-path ae32e5ce3d..825f4a64e612dab62cd0a73b2afe32dac5e0c69f^ | xargs git show  | grep "\<GetArg\>"  | grep '^[+-]' -c
477

There's 477 particular usage of GetArg and even assuming that 50% of this changes are already backported, it is still huge amount.

It means that all trivial changes such as this https://github.com/bitcoin/bitcoin/pull/21056/files

+    const int timeout = gArgs.GetArg("-rpcwaittimeout", DEFAULT_WAIT_CLIENT_TIMEOUT);
+    const int64_t deadline = GetTime<std::chrono::seconds>().count() + timeout;

will require one extra manual conflict resolving/compilation fix.

@kwvg may I ask you to don't backports any big refactorings from v22 or later until the gap is so long?

knst · 2023-08-24T09:12:06Z

merge https://github.com/bitcoin/bitcoin/pull/22951[: move amount.h into consensus](05b355e)

I am also against of this backport by same reason at current moment. It is too early to do; benefits of this backport is too tiny. If there's any critical fix/improvement that blocks other backport, feature or changes - it would out-weight disadvantages. But for bitcoin#22951 and bitcoin#22976 I see only disadvantages.

UdjinM6

utACK

knst

These backports LGTM: bitcoin#24714, bitcoin#21059, bitcoin#23994, bitcoin#19866, bitcoin#19935, bitcoin#23756, bitcoin#19064, bitcoin#23411, bitcoin#20080 (all except 23842)

https://github.com/bitcoin/bitcoin/pull/23842/files
there's missing changes for:

src/dummywallet.cpp
src/qt/test/wallettests.cpp
src/qt/test/addressbooktests.cpp

And they are missing not because they are forgotten, but because there huge refactoring that seems preemptive to do for me.

It's almost last PR from bitcoin v23 but a coverage of backports for v21, v22 and v23 are 62.33%, 39.32%, 16.74%

I don't feel like it is a time to do it :D especially because it's a scripted refactoring (rename of interfaces/variables) - no any new feature, fixes or improvements.

btw, utACK (whatever with 23842 or without, because seems as amount of possible conflicts in future is manageable few)

kwvg · 2023-08-28T06:47:24Z

I don't feel like it is a time to do it :D especially because it's a scripted refactoring (rename of interfaces/variables) - no any new feature, fixes or improvements.

Wasn't a fan of it either myself (had to change my other pull requests to match this newer convention as I've rebased them on top of squashed versions of this PR) but subsequent feature backports assume this new naming convention and I have to therefore pick between changing them now or modifying the backport to use the old naming convention and since the changes were imo manageable, the latter seemed like an unattractive option.

It's almost last PR from bitcoin v23 but a coverage of backports for v21, v22 and v23 are 62.33%, 39.32%, 16.74%

Backporting to ensure feature parity means skipping ahead at times. That is never fun from the perspective of backporting chronologically. Feature parity attempts to make up for the lag in upstream parity by selectively backporting the most impactful segments of the codebase (and is sometimes forced upon downstream projects if features are depending on projects that are deprecating components said features require)

It does help to somewhat close the gap but, is a balancing act between being as prolific as possible and making sure that chronological backports don't become painful.

knst

as I mentioned before:

btw, utACK (whatever with 23842 or without, because seems as amount of possible conflicts in future is manageable few)

PastaPastaPasta · 2023-08-30T02:54:55Z

subsequent feature backports

Which feature backports require 23842?

github-actions · 2023-08-30T03:05:38Z

This pull request has conflicts, please rebase.

kwvg · 2023-08-30T15:52:31Z

Rebased on top of develop (3507b4a) to resolve conflicts

UdjinM6

re-utACK

@kwvg

Seems as old command `git merge-base --fork-point` doesn't work for some cases of tree structure Particularly it didn't worked for PR dashpay#5546: > @kwvg kwvg force-pushed the utilbps branch from 82d8a61 to f1bc1c3 Firstly, `git merge-base --fork-point develop 82d8a61` returned me wrong commit. After pulling origin/develop it returns just an empty string. But `git show-branch --merge-base develop 82d8a61` worked and returned correct commit: "3e1c6dd731 fix: reorder initializations (dashpay#5545)" which is the last commit for develop

knst

re-utACK

PastaPastaPasta

utACK for merging via merge commit

…alletLoader

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

…echnique in util/fastrange.h

… new ones

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

…ting snapshot

…paths

@kwvg

## Issue being fixed or feature implemented Seems as old command `git merge-base --fork-point` doesn't work for some cases of tree structure Particularly it didn't worked for PR #5546: > @kwvg kwvg force-pushed the utilbps branch from 82d8a61 to f1bc1c3 Basically, one of the step `gfb` script -- `git merge-base --fork-point develop 82d8a61` returned me wrong commit. After pulling origin/develop it returns just an empty string. But `git show-branch --merge-base develop 82d8a61` worked and returned correct commit: "3e1c6dd731 fix: reorder initializations (#5545)" which is the last common commit with develop branch ## What was done? Updated recommended function `function gfd()` in doc ## How Has This Been Tested? Tested on PR #5546 that failed before and returned wrong diff for me ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] 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

@knst

…oin{ClientQueueManager,Server} (#5337) ## Motivation CoinJoin's subsystems are initialized by variables and managers that occupy the global context. The _extent_ to which these subsystems entrench themselves into the codebase is difficult to assess and moving them out of the global context forces us to enumerate the subsystems in the codebase that rely on CoinJoin logic and enumerate the order in which components are initialized and destroyed. Keeping this in mind, the scope of this pull request aims to: * Reduce the amount of CoinJoin-specific entities present in the global scope * Make the remaining usage of these entities in the global scope explicit and easily searchable ## Additional Information * The initialization of `CCoinJoinClientQueueManager` is dependent on blocks-only mode being disabled (which can be alternatively interpreted as enabling the relay of transactions). The same applies to `CBlockPolicyEstimator`, which `CCoinJoinClientQueueManager` depends. Therefore, `CCoinJoinClientQueueManager` is only initialized if transaction relaying is enabled and so is its scheduled maintenance task. This can be found by looking at `init.cpp` [here](https://github.com/dashpay/dash/blob/93f8df1c31fdce6a14f149acfdff22678c3f22ca/src/init.cpp#L1681-L1683), [here](https://github.com/dashpay/dash/blob/93f8df1c31fdce6a14f149acfdff22678c3f22ca/src/init.cpp#L2253-L2255) and [here](https://github.com/dashpay/dash/blob/93f8df1c31fdce6a14f149acfdff22678c3f22ca/src/init.cpp#L2326-L2327). For this reason, `CBlockPolicyEstimator` is not a member of `CJContext` and its usage is fulfilled by passing it as a reference when initializing the scheduling task. * `CJClientManager` has not used `CConnman` or `CTxMemPool` as `const` as existing code that is outside the scope of this PR would cast away constness, which would be unacceptable. Furthermore, some logical paths are taken that will grind to a halt if they are stored as `const`. Examples of such a call chains would be: * `CJClientManager::DoMaintenance > CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating > CCoinJoinClientSession::DoAutomaticDenominating > CCoinJoinClientSession::StartNewQueue > CConnman::AddPendingMasternode` which modifies `CConnman::vPendingMasternodes`, which is non-const behaviour * `CJClientManager::DoMaintenance > CCoinJoinClientManager::DoMaintenance > DoAutomaticDenominating > CCoinJoin::IsCollateralValid > AcceptToMemoryPool` which adds a transaction to the memory pool, which is non-const behaviour * There were cppcheck [linter failures](#5337 (comment)) that seemed to be caused by the usage of `Assert` in `coinjoin/client.h`. This seems to be resolved by backporting [bitcoin#24714](bitcoin#24714). (Thanks @knst!) * Depends on #5546 --------- Co-authored-by: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

kwvg mentioned this pull request Aug 24, 2023

refactor: subsume CoinJoin objects under CJContext, deglobalize coinJoin{ClientQueueManager,Server} #5337

Merged

kwvg force-pushed the utilbps branch from 211d2f4 to 8092796 Compare August 24, 2023 08:26

kwvg force-pushed the utilbps branch 4 times, most recently from fc6d450 to 05b355e Compare August 24, 2023 08:36

kwvg force-pushed the utilbps branch from 05b355e to 568a896 Compare August 24, 2023 09:45

kwvg force-pushed the utilbps branch 2 times, most recently from 7ea387e to 82d8a61 Compare August 26, 2023 09:54

kwvg marked this pull request as ready for review August 26, 2023 10:48

kwvg requested review from PastaPastaPasta, UdjinM6 and knst August 26, 2023 11:10

UdjinM6 previously approved these changes Aug 26, 2023

View reviewed changes

UdjinM6 added this to the 20 milestone Aug 26, 2023

knst reviewed Aug 27, 2023

View reviewed changes

kwvg requested a review from knst August 28, 2023 06:48

knst previously approved these changes Aug 29, 2023

View reviewed changes

github-actions bot added the needs rebase label Aug 30, 2023

kwvg dismissed stale reviews from knst and UdjinM6 via f1bc1c3 August 30, 2023 15:51

kwvg force-pushed the utilbps branch from 82d8a61 to f1bc1c3 Compare August 30, 2023 15:51

github-actions bot removed the needs rebase label Aug 30, 2023

kwvg requested review from UdjinM6 and knst August 30, 2023 15:52

UdjinM6 approved these changes Aug 31, 2023

View reviewed changes

knst mentioned this pull request Sep 1, 2023

fix: improve gfd script to make it works for more cases. #5561

Merged

5 tasks

knst approved these changes Sep 1, 2023

View reviewed changes

UdjinM6 mentioned this pull request Sep 1, 2023

fix: add missing includes and remove obsolete includes #5562

Merged

5 tasks

PastaPastaPasta approved these changes Sep 5, 2023

View reviewed changes

kwvg and others added 11 commits September 4, 2023 20:50

merge bitcoin#23842: Rename interfaces::WalletClient to interfaces::W…

c2014e4

…alletLoader

merge bitcoin#24714: Don't use a lambda for Assert/Assume

0761496

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

merge bitcoin#21059: Drop boost/preprocessor dependencies

1cf996a

merge bitcoin#23994: Consolidate all uses of the fast range mapping t…

772a3af

…echnique in util/fastrange.h

merge bitcoin#19866: eBPF Linux tracepoints

e5445e5

merge bitcoin#19935: Move SaltedHashers to separate file and add some…

fa1a5d2

… new ones

merge bitcoin#23756: Fix implicit integer sign changes in strencodings

8c742e9

merge bitcoin#19064: Cleanup thread ctor calls

a2dcf74

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

merge bitcoin#23411: Avoid integer overflow in ApplyStats when activa…

9964cbe

…ting snapshot

args: replace leftover boolean GetArg calls with GetBoolArg

18514d3

merge bitcoin#20080: Strip any trailing / in -datadir and -blocksdir …

72396d5

…paths

PastaPastaPasta force-pushed the utilbps branch from f1bc1c3 to 72396d5 Compare September 5, 2023 01:50

PastaPastaPasta merged commit 0d6281c into dashpay:develop Sep 5, 2023

knst mentioned this pull request Sep 24, 2024

backport: bitcoin#23590, 23061, 23694 #6256

Merged

backport: merge bitcoin#23842, #24714, #21059, #23994, #19866, #19935, #23756, #19064, #23411, #20080 (util backports) #5546

backport: merge bitcoin#23842, #24714, #21059, #23994, #19866, #19935, #23756, #19064, #23411, #20080 (util backports) #5546

Uh oh!

Conversation

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

Uh oh!

Additional Information

Uh oh!

knst commented Aug 24, 2023

Uh oh!

knst commented Aug 24, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwvg commented Aug 28, 2023

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Aug 30, 2023

Uh oh!

github-actions bot commented Aug 30, 2023

Uh oh!

kwvg commented Aug 30, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst 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 24, 2023 •

edited

Loading

knst left a comment •

edited

Loading