Move `CopyrightHolders()` and `LicenseInfo()` into `libbitcoin_common` #26688

hebasto · 2022-12-12T15:36:25Z

After #26645, this change is required to be able to #include clientversion.h in the libbitcoinconsensus code without dependency on util/translation.h.

Steps to reproduce the problem

To demonstrate the problem, let's consider the x86_64-w64-mingw32 platform as it guarantees that all symbols are defined in a library:

$ make -C depends HOST=x86_64-w64-mingw32 NO_QT=1 NO_WALLET=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1
$ ./autogen.sh && ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site && make clean

And now, let's apply a diff as follows:

--- a/src/script/bitcoinconsensus.cpp
+++ b/src/script/bitcoinconsensus.cpp
@@ -8,6 +8,7 @@
 #include <primitives/transaction.h>
 #include <pubkey.h>
 #include <script/interpreter.h>
+#include <util/check.h>
 #include <version.h>
 
 namespace {
@@ -25,7 +26,7 @@ public:
     void read(Span<std::byte> dst)
     {
         if (dst.size() > m_remaining) {
-            throw std::ios_base::failure(std::string(__func__) + ": end of data");
+            throw std::ios_base::failure(STR_INTERNAL_BUG("end of data"));
         }
 
         if (dst.data() == nullptr) {

The resulted code fails to compile:

$ make
...
  CXXLD    libbitcoinconsensus.la
/usr/bin/x86_64-w64-mingw32-ld: script/.libs/libbitcoinconsensus_la-bitcoinconsensus.o:bitcoinconsensus.cpp:(.text+0x1db): undefined reference to `StrFormatInternalBug[abi:cxx11](char const*, char const*, int, char const*)'
...

Obviously, the util/check.cpp needs to be added to the library sources:

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -972,6 +972,7 @@ lib_LTLIBRARIES += $(LIBBITCOINCONSENSUS)
 
 include_HEADERS = script/bitcoinconsensus.h
 libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_base_la_SOURCES) $(libbitcoin_consensus_a_SOURCES)
+libbitcoinconsensus_la_SOURCES += util/check.cpp
 
 libbitcoinconsensus_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS)
 libbitcoinconsensus_la_LIBADD = $(LIBSECP256K1)

Now, a linker complains about another symbol:

$ make
...
  CXXLD    libbitcoinconsensus.la
/usr/bin/x86_64-w64-mingw32-ld: util/.libs/libbitcoinconsensus_la-check.o:check.cpp:(.text+0x8c): undefined reference to `FormatFullVersion[abi:cxx11]()'
...

which is defined in the clientversion.cpp:

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -972,6 +972,7 @@ lib_LTLIBRARIES += $(LIBBITCOINCONSENSUS)
 
 include_HEADERS = script/bitcoinconsensus.h
 libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_base_la_SOURCES) $(libbitcoin_consensus_a_SOURCES)
+libbitcoinconsensus_la_SOURCES += clientversion.cpp util/check.cpp
 
 libbitcoinconsensus_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS)
 libbitcoinconsensus_la_LIBADD = $(LIBSECP256K1)

This time a linker error is:

$ make
...
  CXXLD    libbitcoinconsensus.la
/usr/bin/x86_64-w64-mingw32-ld: .libs/libbitcoinconsensus_la-clientversion.o:clientversion.:(.rdata$.refptr._Z17G_TRANSLATION_FUNB5cxx11[.refptr._Z17G_TRANSLATION_FUNB5cxx11]+0x0): undefined reference to `G_TRANSLATION_FUN[abi:cxx11]'
...

which is supposed to be solved by this PR.

See #26504.

The last time those functions were moved in #24409.

DrahtBot · 2022-12-12T15:36:28Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Ignored review	MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27150 (Deduplicate bitcoind and bitcoin-qt init code by ryanofsky)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake · 2022-12-12T15:40:10Z

  CXX      libbitcoin_node_a-pow.o
  CXX      libbitcoin_node_a-rest.o
bitcoind.cpp:12:10: fatal error: common/license.h: No such file or directory
 #include <common/license.h>
          ^~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [Makefile:15519: bitcoind-bitcoind.o] Error 1

After bitcoin#26645, this change is required to be able to `#include clientversion.h` in the `libbitcoinconsensus` code without dependency on `util/translation.h`.

maflcko · 2022-12-12T16:01:35Z

What are the steps to test/reproduce? I tried, but failed with:

$ git diff  src/
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 38bb11aad4..43084a862f 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -11,6 +11,7 @@
 #include <pubkey.h>
 #include <script/script.h>
 #include <uint256.h>
+#include <util/check.h>
 
 typedef std::vector<unsigned char> valtype;
 
@@ -400,6 +401,8 @@ static bool EvalChecksig(const valtype& sig, const valtype& pubkey, CScript::con
         // Key path spending in Taproot has no script, so this is unreachable.
         break;
     }
+     STR_INTERNAL_BUG("");
+     NONFATAL_UNREACHABLE();
     assert(false);
 }

See bitcoin#26688 (comment)

hebasto · 2022-12-12T16:34:46Z

@MarcoFalke

What are the steps to test/reproduce? I tried, but failed with:

An additional diff for your test is required as follows:

--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -973,7 +973,7 @@ if BUILD_BITCOIN_LIBS
 lib_LTLIBRARIES += $(LIBBITCOINCONSENSUS)
 
 include_HEADERS = script/bitcoinconsensus.h
-libbitcoinconsensus_la_SOURCES = support/cleanse.cpp $(crypto_libbitcoin_crypto_base_la_SOURCES) $(libbitcoin_consensus_a_SOURCES)
+libbitcoinconsensus_la_SOURCES = clientversion.cpp support/cleanse.cpp util/check.cpp $(crypto_libbitcoin_crypto_base_la_SOURCES) $(libbitcoin_consensus_a_SOURCES)
 
 libbitcoinconsensus_la_LDFLAGS = $(AM_LDFLAGS) -no-undefined $(RELDFLAGS)
 libbitcoinconsensus_la_LIBADD = $(LIBSECP256K1)

See bitcoin#26688 (comment)

maflcko · 2022-12-12T16:39:08Z

Ok, but what is the diff/steps to test that diff? ;)

hebasto · 2022-12-12T16:40:13Z

What are the steps to test...?

Perhaps, the first two commits from #26504 are.

maflcko · 2022-12-13T08:09:46Z

26504 is quite large. It would help testers if there was a MWE (minimal (non)working example) available.

So for now, I tend toward NACK, unless there is a reason to do this change, with an explanation or steps to verify.

maflcko · 2022-12-15T17:44:47Z

26504 compiles fine for me with the changes here reverted

hebasto · 2022-12-15T20:01:27Z

@MarcoFalke

26504 compiles fine for me with the changes here reverted

And cross-compiling for Windows as well?

maflcko · 2022-12-15T20:11:44Z

Haven't tried that. I think it would be helpful to mention it in the description, so that testing is easier.

maflcko · 2022-12-15T21:06:03Z

Ok, I used my diff to test (#26688 (comment)) on FILE_ENV="./ci/test/00_setup_env_win64.sh" and could reproduce the build failure. However, I needed #26688 (comment) as well to pass, so I'd suggest adding it here. Otherwise the fix seems incomplete.

hebasto · 2022-12-17T10:29:37Z

Haven't tried that. I think it would be helpful to mention it in the description, so that testing is easier.

The PR description has been updated. A set of diffs was introduced to reproduce the problem and test its fix easily. Specifically noted, that using of x86_64-w64-mingw32 platform makes the problem recognizable as early as possible.

However, I needed #26688 (comment) as well to pass, so I'd suggest adding it here. Otherwise the fix seems incomplete.

Sorry, but I disagree. Adding source files, e.g., clientversion.cpp and util/check.cpp, is required to define symbols due to adding a new code, e.g., STR_INTERNAL_BUG(), to the library. This PR is focused on moving of the piece of code into a new source file, and it adds no code to the library.

maflcko

Ok, lgtm

hebasto · 2022-12-20T08:42:31Z

Friendly ping @fanquake :)

fanquake · 2022-12-20T10:31:38Z

It's not clear from the PR description why we are making this change. The description says that if you apply a diff, the build fails. However it doesn't explain why we are fixing a problem we don't have; there are currently no build issues with master, only after you apply a diff to (purposefully) break things.

You say

See #26504.

but it's not clear there is consensus on making that change, and the PR description here doesn't explain how this a prerequisite for that, or, if so, why it needs to be split out in advance.

It would be good for the PR description to either:

Explain why, stand-alone, we should make this change to master. Maybe this code should just live in lib common instead of lib util? If so, why?
Explain that it's a requirement for 26504, and explain why we need to shuffle code now, even though nothing is broken, and it's not clear that 26504 will actually go ahead.

maflcko · 2022-12-20T15:39:10Z

apply a diff to (purposefully) break things.

I don't think using util/check in code is a malicious change? At some point util/check will be used in libbitcoinconsensus, after which this change is required, so might as well do it now, to avoid putting the burden on an unsuspecting future dev?

An alternative, I presume, would be to inject G_TRANSLATION_FUN into libbitcoinconsensus, but I don't know if that makes more sense.

fanquake · 2022-12-20T15:58:15Z

I don't think using util/check in code is a malicious change?

I also don't think it's malicious. My point is just that if the entire PR rationale is that the build is broken, but only after you've broken it with some other patch (i.e not master), I don't think that's great reasoning to justify a refactor.

maflcko · 2022-12-20T16:04:19Z

I wouldn't say it is "broken" to use util/check. It seems that it should be supported in all code.

DrahtBot · 2023-03-07T18:06:42Z

🐙 This pull request conflicts with the target branch and needs rebase.

maflcko · 2023-04-26T14:26:45Z

I think this is a bugfix/requirement for #26504, so I am not sure why it was closed but the other not?

hebasto · 2023-04-27T06:15:52Z

I think this is a bugfix/requirement for #26504, so I am not sure why it was closed but the other not?

It is closed as well now.

hebasto mentioned this pull request Dec 12, 2022

Add UNREACHABLE macro #26504

Closed

Move CopyrightHolders() and LicenseInfo() into libbitcoin_common

1370d89

After bitcoin#26645, this change is required to be able to `#include clientversion.h` in the `libbitcoinconsensus` code without dependency on `util/translation.h`.

hebasto force-pushed the 221212-common branch from 1b176d1 to 1370d89 Compare December 12, 2022 15:44

hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 12, 2022

Test commit

c3bc03c

See bitcoin#26688 (comment)

hebasto added a commit to hebasto/bitcoin that referenced this pull request Dec 12, 2022

Test commit

b794cab

See bitcoin#26688 (comment)

maflcko approved these changes Dec 17, 2022

View reviewed changes

maflcko requested a review from fanquake December 17, 2022 11:33

DrahtBot mentioned this pull request Jan 21, 2023

Add fee_est tool for debugging fee estimation code #10443

Closed

hebasto mentioned this pull request Feb 2, 2023

build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set #25972

Merged

DrahtBot mentioned this pull request Feb 24, 2023

Deduplicate bitcoind and bitcoin-qt init code #27150

Merged

DrahtBot added the Needs rebase label Mar 7, 2023

achow101 closed this Apr 25, 2023

bitcoin locked and limited conversation to collaborators Apr 26, 2024

Move CopyrightHolders() and LicenseInfo() into libbitcoin_common #26688

Move CopyrightHolders() and LicenseInfo() into libbitcoin_common #26688

Uh oh!

Conversation

hebasto commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to reproduce the problem

Uh oh!

DrahtBot commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

fanquake commented Dec 12, 2022

Uh oh!

maflcko commented Dec 12, 2022

Uh oh!

hebasto commented Dec 12, 2022

Uh oh!

maflcko commented Dec 12, 2022

Uh oh!

hebasto commented Dec 12, 2022

Uh oh!

maflcko commented Dec 13, 2022

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

hebasto commented Dec 15, 2022

Uh oh!

maflcko commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Dec 15, 2022

Uh oh!

hebasto commented Dec 17, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 20, 2022

Uh oh!

fanquake commented Dec 20, 2022

Uh oh!

maflcko commented Dec 20, 2022

Uh oh!

fanquake commented Dec 20, 2022

Uh oh!

maflcko commented Dec 20, 2022

Uh oh!

DrahtBot commented Mar 7, 2023

Uh oh!

maflcko commented Apr 26, 2023

Uh oh!

hebasto commented Apr 27, 2023

Uh oh!

Uh oh!

Move `CopyrightHolders()` and `LicenseInfo()` into `libbitcoin_common` #26688

Move `CopyrightHolders()` and `LicenseInfo()` into `libbitcoin_common` #26688

hebasto commented Dec 12, 2022 •

edited

Loading

DrahtBot commented Dec 12, 2022 •

edited

Loading

maflcko commented Dec 15, 2022 •

edited

Loading