Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jun 16, 2020

FatalError has been copied from AbortNode, so the two should use the same style to avoid confusion.

Follow-up to #18927

This is needed for consistency with AbortNode
@hebasto
Copy link
Member

hebasto commented Jun 16, 2020

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK fa02b47, I have reviewed the code and it looks OK, I agree it can be merged.

Suggestions for followups:

  • remove "debug.log" from the translated string
  • replace "debug.log" with the actual log file name (-debuglogfile option)


/** Show error message **/
bool InitError(const bilingual_str& str);
constexpr auto AbortError = InitError;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this header selected to host this expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be in a header because it is used in two places. Any suggestions where to put it?

Copy link
Member

Choose a reason for hiding this comment

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

I meant "why in this header" rather "why in header" :)
This header looks good to me. Another possible one is validation.h.

Copy link
Member

Choose a reason for hiding this comment

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

But your variant is better for a code reader, I think.

@maflcko maflcko merged commit 39bd9dd into bitcoin:master Jun 17, 2020
@maflcko maflcko deleted the 2006-refactorAbortError branch June 17, 2020 10:43
@Sjors
Copy link
Member

Sjors commented Jun 17, 2020

As @achow101 noticed on IRC, this PR appears to cause the following error on Ubuntu 20.04, but not on macOS. Apparently only with --enable-debug.

./configure --enable-debug --with-incompatible-bdb --enable-werror
make
/usr/bin/ld: bitcoin_wallet-bitcoin-wallet.o:(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
/usr/bin/ld: libbitcoin_wallet_tool.a(libbitcoin_wallet_tool_a-wallettool.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
/usr/bin/ld: libbitcoin_wallet.a(libbitcoin_wallet_a-salvage.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
/usr/bin/ld: libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
/usr/bin/ld: libbitcoin_wallet.a(libbitcoin_wallet_a-walletdb.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
/usr/bin/ld: libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): more undefined references to `InitError(bilingual_str const&)' follow
collect2: error: ld returned 1 exit status

@jonatack
Copy link
Member

I'm seeing the issue on Debian as well.

$ uname -a
Linux 4.19.0-9-amd64 #1 SMP Debian 4.19.118-2+deb10u1 (2020-06-07) x86_64 GNU/Linux

@hebasto
Copy link
Member

hebasto commented Jun 17, 2020

@achow101 @Sjors @jonatack Mind testing #19309?

maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Jun 19, 2020
b83cc0f Fix link error with --enable-debug (Hennadii Stepanov)

Pull request description:

  Fixes a link error on master (39bd9dd):
  ```
  $ ./configure --enable-debug
  $ make
  ...
  bitcoin_wallet-bitcoin-wallet.o:(.data.rel.ro+0x0): undefined reference to `InitError(bilingual_str const&)'
  libbitcoin_wallet_tool.a(libbitcoin_wallet_tool_a-wallettool.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
  libbitcoin_wallet.a(libbitcoin_wallet_a-salvage.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
  libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
  libbitcoin_wallet.a(libbitcoin_wallet_a-walletdb.o):(.data.rel.ro+0x8): undefined reference to `InitError(bilingual_str const&)'
  libbitcoin_wallet.a(libbitcoin_wallet_a-wallet.o):(.data.rel.ro+0x8): more undefined references to `InitError(bilingual_str const&)' follow
  collect2: error: ld returned 1 exit status
  ```

  See:
  - bitcoin/bitcoin#19295 (comment)
  - bitcoin/bitcoin#19295 (comment)

ACKs for top commit:
  achow101:
    Re-ACK b83cc0f

Tree-SHA512: f563d978b6725284049449bb0b3a184d356f32e9b63bcadb0ba71352d3d521af3dbb4a7b4fc0a5a620ed99c357e59f62249c10d0defc0cbe7775f2c06791dabe
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Dec 11, 2020
Summary:
```
FatalError has been copied from AbortNode, so the two should use the
same style to avoid confusion.

Follow-up to #18927
```

Backport of core [[bitcoin/bitcoin#19295 | PR19295]], [[bitcoin/bitcoin#19309 | PR19309]] and [[bitcoin/bitcoin#19331 | PR19331]].
[[bitcoin/bitcoin#19295 | PR19295]] is the original intent
[[bitcoin/bitcoin#19309 | PR19309]] is a hacky bug fix
[[bitcoin/bitcoin#19331 | PR19331]] reverts [[bitcoin/bitcoin#19309 | PR19309]] and fixes [[bitcoin/bitcoin#19295 | PR19295]].

In the end the relevant commits are:
bitcoin/bitcoin@fa02b47
bitcoin/bitcoin@fac96e6
bitcoin/bitcoin@fa72ca6
bitcoin/bitcoin@cccc278

Depends on D8649.

Test Plan:
With debug:
  ninja all check-all

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D8650
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants