Skip to content

Backport 14734, 15457, 16344, 16352, 16366 #4532

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
Closed
PastaPastaPasta wants to merge 5 commits into from
Closed

Backport 14734, 15457, 16344, 16352, 16366 #4532

PastaPastaPasta wants to merge 5 commits into from

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Oct 22, 2021

No description provided.

PastaPastaPasta and others added 5 commits October 22, 2021 00:16
@PastaPastaPasta
Merge bitcoin#14734: fix an undefined behavior in uint::SetHex
3daeb28 
0f459d8 fix an undefined behavior in uint::SetHex (Kaz Wesley)

Pull request description:

  Decrementing psz beyond the beginning of the string is UB, even though
  the out-of-bounds pointer is never dereferenced.

  I don't think any clang sanitizer covers this, so I don't see any way a test could catch the original behavior.

ACKs for top commit:
  promag:
    utACK 0f459d8.
  l2a5b1:
    utACK 0f459d8

Tree-SHA512: 388223254ea6e955f643d2ebdf74d15a3d494e9f0597d9f05987ebb708d7a1cc06ce64bd25d447d75b5f5561bdae9630dcf25adb7bd75f7a382298b95d127162

# Conflicts:
#	src/uint256.cpp
@laanwj @PastaPastaPasta
Merge bitcoin#15457: Check std::system for -[alert|block|wallet]notify
56251ec 
f874e14 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb [build] detect std::system or ::wsystem (Sjors Provoost)

Pull request description:

  Platforms such as iOs and Universal Windows Platform do not support launching a process through system().

ACKs for top commit:
  laanwj:
    code review ACK f874e14

Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d

# Conflicts:
#	build_msvc/bitcoin_config.h
#	src/init.cpp
#	src/wallet/init.cpp
@fanquake @PastaPastaPasta
Merge bitcoin#16344: build: use #if HAVE_SYSTEM instead of defined(HA…
79e5eb8 
…VE_SYSTEM)

976b034 [build]: use #if HAVE_SYSTEM instead of defined(HAVE_SYSTEM) (Sjors Provoost)

Pull request description:

  It seems that `AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM]` causes `HAVE_SYSTEM` to always be defined, so we need to use `#if HAVE_SYSTEM` instead of `#if defined(HAVE_SYSTEM)`.

  Followup for bitcoin#15457, can be tested with bitcoin#12557.

ACKs for top commit:
  dongcarl:
    ACK bitcoin@976b034.
  promag:
    ACK 976b034.
  fanquake:
    ACK 976b034

Tree-SHA512: b8cdd04c2ec399fd15638aef5d75ea0886ec1572d3cf4fcea27c193e1e6390344315908262cad8981a9b0a905ab9520619ce2ffe9a717f4ee6bfa8b028ebbdc6

# Conflicts:
#	src/init.cpp
@laanwj @PastaPastaPasta
Merge bitcoin#16352: build: prune dbus from depends
6d6b585 
e8fabd9 build: prune dbus from depends (fanquake)

Pull request description:

  Since bitcoin#8210 (59d063d), we've been passing `-dbus-runtime` when configuring Qt.

  ```
  qtbase-opensource-src-5.9.7 $ ./configure -h | grep -i dbus
    -no-dbus ............. Do not build the Qt D-Bus module
    -dbus-linked ......... Build Qt D-Bus and link to libdbus-1 [auto]
    -dbus-runtime ........ Build Qt D-Bus and dynamically load libdbus-1 [no]
  ```

  This means we don't actually seem to be using the `D-Bus` we build in depends. This was pointed out by theuni at the time, [here](bitcoin#7993 (comment)) and [here](bitcoin#8210 (comment)), but was never followed up. dongcarl also bought it up as part of bitcoin#16150.

  I've tested building and running `bitcoin-qt` using depends on Debian. Needs further testing.

ACKs for top commit:
  laanwj:
    code review ACK e8fabd9

Tree-SHA512: 164e6e52b6f97c04aef42bd185e2a157bc1a42103840f9404c5a795749f45a8c2c35f35873395a3a56398b3cd5955496b90d9c885d929b434c9bc871695abe20

# Conflicts:
#	depends/packages/dbus.mk
#	depends/packages/packages.mk
#	doc/dependencies.md
@PastaPastaPasta
Merge bitcoin#16366: init: Use InitError for all errors in bitcoind/qt
fe14f50 
fa6f402 Call node->initError instead of InitError from GUI code (Russell Yanofsky)
fad2502 init: Use InitError for all errors in bitcoind/qt (MarcoFalke)

Pull request description:

  Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again:

  ```sh
  BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args

ACKs for top commit:
  hebasto:
    ACK fa6f402
  ryanofsky:
    utACK fa6f402. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node `InitError` function and global variables from GUI code.

Tree-SHA512: bd19e08dcea4019dfe40356bc5c63cb583cefed54b6c9dcfb82f1b5b00308d8e2b363549afcaea5e93bf83864dbe0917400c3b70f43a8a5bdff45c9cd34cc294
@PastaPastaPasta PastaPastaPasta added this to the 18 milestone Oct 22, 2021
UdjinM6
UdjinM6 requested changes Oct 22, 2021
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.

should apply 15457 and 16344 to -instantsendnotify too

src/dashd.cpp
if (!gArgs.ReadConfigFiles(error, true)) {
fprintf(stderr, "Error reading configuration file: %s\n", error.c_str());
return false;
return InitError(strprintf("Error reading configuration file: %s\n", error));
Copy link

@UdjinM6 UdjinM6 Oct 22, 2021

Choose a reason for hiding this comment

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

16366: should apply it below too, line 114

src/dashd.cpp
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
#endif
fprintf(stdout, "Dash Core server starting\n");
tfm::format(std::cout, PACKAGE_NAME "daemon starting\n");
Copy link

@UdjinM6 UdjinM6 Oct 22, 2021

Choose a reason for hiding this comment

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

16366: should keep fprintf

src/qt/dash.cpp
SetupUIArgs();
std::string error;
if (!node->parseParameters(argc, argv, error)) {
node->initError(strprintf("Error parsing command line arguments: %s\n", error));
Copy link

@UdjinM6 UdjinM6 Oct 22, 2021

Choose a reason for hiding this comment

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

16366: should probably apply it near QMessageBox::critical for printcrashinfo, various font and css settings below too

@github-actions
Copy link

github-actions bot commented Oct 24, 2021

This pull request has conflicts, please rebase.

@github-actions
Copy link

github-actions bot commented Dec 30, 2021

This Pull Request may conflict if the Pull Requests below are merged first.

#4649
conflictable files: src/interfaces/node.cpp,src/interfaces/node.h

This was referenced Dec 30, 2021
Merged
Merged
@github-actions
Copy link

github-actions bot commented Dec 30, 2021

This Pull Request may conflict if the Pull Requests below are merged first.

#4650
conflictable files: src/validation.cpp

@UdjinM6 UdjinM6 removed this from the 18 milestone Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 requested changes

Assignees

No one assigned

Labels

Bitcoin Backport needs rebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PastaPastaPasta @UdjinM6 @laanwj @fanquake