Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

backport: bitcoin#17946, #18663, #18946, #18975, #19004, #19115, #19152, #19173, partial #18735 #5802

Merged
merged 9 commits into from
Jan 9, 2024
Merged

backport: bitcoin#17946, #18663, #18946, #18975, #19004, #19115, #19152, #19173, partial #18735 #5802

merged 9 commits into from
Jan 9, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jan 7, 2024 •
edited
Loading

Issue being fixed or feature implemented

Just regular bitcoin backports, mostly from v21

What was done?

Note, that 18735 is partial due to different implementation of our and bitcoin's CI, paths are different, scripts are different... Remaining part DNM (do-not-merge) at least currently

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A

Checklist:

  • 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
  • I have assigned this pull request to a milestone

@knst knst added this to the 20.1 milestone Jan 7, 2024
@knst knst force-pushed the bp-v21-p4 branch 2 times, most recently from 9301c09 to 02b0f08 Compare January 8, 2024 12:20
@knst knst changed the title backport: bitcoin#17946, #18152, #18663, #18735, #18946, #18975, #19004, #19115, #19152, #19173 backport: bitcoin#17946, #18152, #18663, #18946, #18975, #19004, #19115, #19152, #19173, partial #18735 Jan 8, 2024
UdjinM6
UdjinM6 reviewed Jan 8, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few questions

src/rpc/mining.cpp Show resolved Hide resolved
src/qt/bitcoingui.cpp Outdated
Comment on lines 1256 to 1260
/*
if (fNetworkBecameActive || fNetworkBecameInactive) {
setNumBlocks(m_node.getNumBlocks(), QDateTime::fromTime_t(m_node.getLastBlockTime()), QString::fromStdString(m_node.getLastBlockHash()), m_node.getVerificationProgress(), false);
}
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18152: ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see commit description e76f696

src/qt/bitcoingui.cpp Outdated
Comment on lines 1500 to 1502
/*
setNumBlocks(m_node.getNumBlocks(), QDateTime::fromTime_t(m_node.getLastBlockTime()), QString::fromStdString(m_node.getLastBlockHash()), m_node.getVerificationProgress(), false);
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

18152: ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see commit description e76f696

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Clarification for setAdditionalDataSyncProgres:
    The call of masternodeSync.Reset() can happen only if chain is destroyed,
    it happens if application is closed. We don't need to update num of blocks
    in this situation, as well we don't need to start spinner

This is not true. We force sync reset in a couple of cases in CMasternodeSync itself and via mnsync reset RPC

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

src/net.cpp:        ::masternodeSync->Reset();
src/net.cpp:    ::masternodeSync->Reset();
src/qt/bitcoingui.cpp:    // If masternodeSync->Reset() has been called make sure status bar shows the correct information.
src/test/util/setup_common.cpp:    ::masternodeSync.reset();
src/rpc/misc.cpp:        ::masternodeSync->Reset(true);
src/init.cpp:    ::masternodeSync.reset();

ah I see now, reset() and Reset() are different things.

Could you suggest a fix for case of Reset()?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we had setNumBlocks here and in (1) because we wanted up to date info so it should be POST_INIT (i.e. disable throttling) in both cases I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I looked to usages of masternodeSync::Reset():

once it is called inside CConnman::SetNetworkActive - so it means that it can be called if network become active/inactive... Rpc mnsync can be called before sync is also finish.

So, I'd prefer to keep it as is in e76f696 now

Copy link

@UdjinM6 UdjinM6 Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing these sync statuses change in setNumBlocks is app nap state switch and we definitely do not want app nap to be enabled in both cases. POST_INIT is probably not a safe one then, should pick any other state, doesn't really matter which one exactly but INIT_DOWNLOAD is probably the less confusing one.

::masternodeSync.reset() can only happen on shutdown and anything would work there imo, I would just ignore this case.

So, I'd prefer to keep it as is in e76f696 now

I disagree, we should re-enable setNumBlocks and the spnner.

Copy link

@UdjinM6 UdjinM6 Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we maybe drop 18152 from this PR and do it in a separate one or will it cause some merge conflicts?

EDIT: just tried it myself, looks like it can be dropped safely

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here's it: #5804

no conflicts, but 18152 is needed for several further backports from v21/v22 (no PRs for them yet)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: just tried it myself, looks like it can be dropped safely

yes, I slightly tested before PR is created: run app, stop app before sync is over; restart and wait until sync is over; call re-index; also checked spinner appearance after connection to peers are established.

I didn't check all cases, but looks like main-flow works correctly with e76f696

@knst knst requested a review from UdjinM6 January 8, 2024 18:14
@knst knst marked this pull request as draft January 8, 2024 18:52
@knst knst force-pushed the bp-v21-p4 branch 2 times, most recently from fa5919a to 07a76ed Compare January 8, 2024 19:26
@knst knst mentioned this pull request Jan 8, 2024
Merged
5 tasks
UdjinM6
UdjinM6 approved these changes Jan 8, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, utACK

@UdjinM6 UdjinM6 changed the title backport: bitcoin#17946, #18152, #18663, #18946, #18975, #19004, #19115, #19152, #19173, partial #18735 backport: bitcoin#17946, #18663, #18946, #18975, #19004, #19115, #19152, #19173, partial #18735 Jan 8, 2024
@knst
Copy link
Collaborator Author

knst commented Jan 9, 2024

there's no more conflict between #5767 and this PR due to moved backport bitcoin#18152 to new PR.

@knst knst marked this pull request as ready for review January 9, 2024 13:49
PastaPastaPasta
PastaPastaPasta approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for merging via merge commit

MarcoFalke and others added 9 commits January 9, 2024 08:13
@PastaPastaPasta
Merge bitcoin#18663: doc: mention build docs in README.md
30c1a2e 
bda62e8 Adding build instructions to Bitcoin Core, fixes bitcoin#18658 (Saahil Shangle)

Pull request description:

  Making the instructions for building Bitcoin Core more clear in the main `README.md` will reduce confusion between the `build_msvc` and `doc` folders.

ACKs for top commit:
  laanwj:
    ACK bda62e8

Tree-SHA512: ee4c394661eba48d4229e3d1e9ddb67ccb79589429bfa9986cb0242cd615d1f3cc5332063562c1e89c0cdd9ae2e609f61e8bfb209926d8363d35d3da6d94ae9c
@laanwj @PastaPastaPasta
Merge bitcoin#18946: rpcwallet: Replace boost::optional<T>::emplace w…
d2a2054 
…ith simple assignment of T{}

fa1f840 rpcwallet: Replace pwallet-> with wallet. (MarcoFalke)
fa182a8 rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} (MarcoFalke)

Pull request description:

  Closes bitcoin#18943

ACKs for top commit:
  laanwj:
    ACK fa1f840
  ryanofsky:
    Code review ACK fa1f840 and thanks for using a standalone commit for the fix
  promag:
    Code review ACK fa1f840.
  hebasto:
    ACK fa1f840, tested on Linux Mint 19.3.

Tree-SHA512: 0838485d1f93f737ce5bf12740669dcafeebb78dbc3fa15dbcc511edce64bf024f60f0497a04149a1e799d893d57b0c9ffe442020c1b9cfc3c69db731f50e712
@fanquake @PastaPastaPasta
Merge bitcoin#18975: test: Remove const to work around compiler error…
41dcede 
… on xenial

050e2ee test: Remove const to work around compiler error on xenial (Wladimir J. van der Laan)

Pull request description:

  Fix the following error in travis:

      test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor

      const BlockValidationState state_dummy;

ACKs for top commit:
  MarcoFalke:
    Tested ACK 050e2ee on xenial with clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
  fanquake:
    ACK 050e2ee - I see why we didn't hit this on master. We are installing the `clang-8` packages for the tsan job. However on the 0.20 branch we are still just installing `clang`, which is 3.8.

Tree-SHA512: 8a1d57289dbe9895ab79f81ca87b4fd723426b8d72f3a34bec9553226fba69f6dc19551c1f1d52db6c4b2652164a02ddc60f3187c3e2ad7bcacb0aaca7fa690a
@PastaPastaPasta
Merge bitcoin#17946: Fix GBT: Restore "!segwit" and "csv" to "rules" key
061f2b5 
412d5fe QA: feature_segwit: Check that template "rules" includes "!segwit" as appropriate (Luke Dashjr)
2abe8cc Bugfix: Include "csv","!segwit" in "rules" (Luke Dashjr)

Pull request description:

  bitcoin#16060 removed CSV & segwit from versionbits, breaking the "rules" key returned by GBT.

  Without this, miners don't know they're mining segwit blocks, and should fall back to pre-segwit block creation.

ACKs for top commit:
  sipa:
    ACK 412d5fe
  jnewbery:
    Tested ACK 412d5fe.

Tree-SHA512: 825d72e257dc0dd4941f2fe498d8d4f4f2a21b9505cd21a8f9eb7fb5d6d7dd9219347928cf90bb57a777920ce24295859763e64fa8a22ebb58fc2380f80f5615
@PastaPastaPasta
Merge bitcoin#19004: refactor: Replace const char* to std::string
f2790a8 
c57f03c refactor: Replace const char* to std::string (Calvin Kim)

Pull request description:

  Rationale: Addresses bitcoin#19000
  Some functions should be returning std::string instead of const char*.
  This commit changes that.

  Main benefits/reasoning:

  1.  The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
  2. All call sites convert to string anyway

ACKs for top commit:
  MarcoFalke:
    re-ACK c57f03c (no changes since previous review) 🚃
  Empact:
    Fair enough, Code Review ACK bitcoin@c57f03c
  practicalswift:
    ACK c57f03c -- patch looks correct
  hebasto:
    re-ACK c57f03c

Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd
@PastaPastaPasta
partial Merge bitcoin#18735: ci: Add and document BASE_BUILD_DIR
5a8df5e 
fae49f6 ci: Add and document BASE_BUILD_DIR (MarcoFalke)

Pull request description:

  Also fixes bitcoin#18768

ACKs for top commit:
  hebasto:
    re-ACK fae49f6, which is essentially the same as the previously [reviewed changes](bitcoin#18735 (review)).

Tree-SHA512: 216565a05ccd513dd9f114b2333d3c283fd71914d32f9b05f145cb7c70633b083ff8ef60798d6f22f4be6a4d652b03806551fd74b5b596c92968501a4d9726d2
@knst @PastaPastaPasta
Merge bitcoin#19115: doc: Add release notes for 17219
1f81fdc
@fanquake @PastaPastaPasta
Merge bitcoin#19152: build: improve build OS configure output
26abf94 
0fef60c build: improved output of configure for build OS (sachinkm77)

Pull request description:

  The purpose of this fix is to improve output of the configure script by providing the build OS. This is done by leveraging the build_os set by the script config.sub / config.guess. bitcoin#18966

ACKs for top commit:
  fanquake:
    ACK 0fef60c - thanks for following up.

Tree-SHA512: b9f49df901a9d37eb16c67c063bb3611602a84391aa54d097a52b740f474c2785c24bf405522d15d724fde25070d354bf20b885add2ee4405a71cbe9ebab5ff3
@PastaPastaPasta
Merge bitcoin#19173: build: turn on --enable-c++17 by --enable-fuzz
d5e709b 
0012471 build: turn on --enable-c++17 by --enable-fuzz (Vasil Dimov)

Pull request description:

  Fuzzing code uses C++17 specific code (e.g. std::optional), so it is not
  possible to compile with --enable-fuzz and without --enable-c++17.

  Thus, turn on --enable-c++17 whenever --enable-fuzz is used.

ACKs for top commit:
  hebasto:
    ACK 0012471, tested on Linux Mint 19.3 (x86_64); verified that it fails to compile with `--enable-fuzz` and without `--enable-c++17` on master.

Tree-SHA512: 290531ea8d79de3b9251ea4ad21e793478b18150cc0124eea1e50c3a4ed92bab89c3e70ed0aa526906f8723ea952cdba4268f1560ae4be9bd25b9e4f9b97436c
@PastaPastaPasta PastaPastaPasta merged commit e8ea36c into dashpay:develop Jan 9, 2024
4 of 5 checks passed
@knst knst deleted the bp-v21-p4 branch January 9, 2024 18:04
@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Milestone
20.1
Development

Successfully merging this pull request may close these issues.
6 participants
@knst @UdjinM6 @PastaPastaPasta @thephez @laanwj @fanquake