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#19522, #19809, #20993, #21075, #21126, #21138, #21221, #21354, #21542 #5974

Merged
merged 11 commits into from
Apr 12, 2024
Merged

backport: bitcoin#19522, #19809, #20993, #21075, #21126, #21138, #21221, #21354, #21542 #5974

merged 11 commits into from
Apr 12, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Apr 6, 2024 •
edited

Issue being fixed or feature implemented

Regular backports from bitcoin v22 and related fixes

What was done?

Follow-up fixes for bitcoin#19809
Backports:

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A
Notice, that function name is included now by default to logs with -logfunctionnames and many logs have function name twice now such as:

 node0 2024-04-06T20:13:56.564123Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.3] [masternode/sync.cpp:331] [NotifyHeaderTip] CMasternodeSync::NotifyHeaderTip -- pindexNew->nHeight: 5 fInitialDownload=0

For further development need to take it in account and do not use more direct calls __func__ from code as well as reduce usages in codebase.

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 21 milestone Apr 6, 2024
PastaPastaPasta
PastaPastaPasta reviewed Apr 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.

general utACK 32ec395; but please rebase

@knst
Copy link
Collaborator Author

knst commented Apr 9, 2024

@knst knst force-pushed the bp-v22-p5 branch from 32ec395 to d4dcdf0

rebased on the top of develop after merging master

UdjinM6
UdjinM6 requested changes Apr 9, 2024
View reviewed changes
src/llmq/commitment.cpp Outdated
@@ -30,7 +30,7 @@ CFinalCommitment::CFinalCommitment(const Consensus::LLMQParams& params, const ui
template<typename... Types>
void LogPrintfFinalCommitment(Types... out) {
if (LogAcceptCategory(BCLog::LLMQ)) {
LogInstance().LogPrintStr(strprintf("CFinalCommitment::%s -- %s", __func__, tinyformat::format(out...)));
LogInstance().LogPrintStr(strprintf(" -- %s", tinyformat::format(out...)), "CFinalCommitment::" __FILE__, __func__, __LINE__);
Copy link

Choose a reason for hiding this comment

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

19809: this produces an awkward output like

[   msghand] [LogPrintfFinalCommitment:33] [CFinalCommitment::llmq/commitment.cpp]  -- CFinalCommitment::Verify mns[3] validMembers[v[0]=1v[1]=1v[2]=1] signers[s[0]=1s[1]=1s[2]=1]
[   msghand] [LogPrintfFinalCommitment:33] [CFinalCommitment::llmq/commitment.cpp]  -- q[2aa24fd0e93a6d47f90a1b0c46d75581b7cc36a52cb3ce1559e339a652af9b4b] VALID

I see 2 issues here:

  1. file name and func name are in the wrong places
  2. with this new option disabled (which is the default state) the second line becomes 
     [   msghand]   -- q[2aa24fd0e93a6d47f90a1b0c46d75581b7cc36a52cb3ce1559e339a652af9b4b] VALID
    which is not super informative imo.

Also, 19809 won't work correctly with lambdas e.g. queueAndMaybePushInv.

Copy link
Collaborator Author

@knst knst Apr 10, 2024 •
edited

Choose a reason for hiding this comment

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

with this new option disabled (which is the default state) the second line becomes

for functional tests the default state "is enabled"
If you are debugging an incident on production, you can find by substring where it come from.
Also please notice, that by default logs are very non-verbose if you haven't specified -debug option, but if you did specify manually -debug option you can specify -logfunctionnames as well.

The particular case with too short unclear message is fixed.

 [   msghand]   -- q[2aa24fd0e93a6d47f90a1b0c46d75581b7cc36a52cb3ce1559e339a652af9b4b] VALID

file name and func name are in the wrong places

It was also a wrong source line and function name LogPrintfFinalCommitment - fixed both.

Also, 19809 won't work correctly with lambdas e.g. queueAndMaybePushInv.

Indeed, it doesn't work with lambdas. So, for lambda you have only source file and source line.
The Log LogPrint(BCLog::NET, "SendMessages -- queued inv: %s index=%d peer=%d\n", invIn.ToString(), vInv.size(), pto->GetId()); produces:

 node0 2024-04-10T07:25:02.547034Z (mocktime: 2014-12-04T17:15:52Z) [   msghand] [net_processing.cpp:5306] [operator()] SendMessages -- queued inv: spork 088c66c8636eff4ab7b082578ec899f8aecf324bc2675659fcc9d001022516ad  index=0 peer=0

which is indeed not beautiful due to operator() instead function name but in this particular case it didn't become worse, because SendMessages is already prefix string in log.

Please review newer version

test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
test/functional/test_framework/test_node.py Outdated Show resolved Hide resolved
@UdjinM6 UdjinM6 mentioned this pull request Apr 9, 2024
Merged
5 tasks
@knst knst requested a review from UdjinM6 April 10, 2024 07:48
@UdjinM6
Copy link

UdjinM6 commented Apr 10, 2024

LGTM but needs rebase to fix ff-only merge check

MarcoFalke and others added 11 commits April 11, 2024 02:25
@knst
Merge bitcoin#21075: doc: Fix markdown formatting
f254f77 
e1604b3 doc: Replace tabs for spaces (Gunar C. Gessner)
98db48d doc: Fix markdown formatting (Gunar Gessner)

Pull request description:

  Lines were being joined making it hard to read.

ACKs for top commit:
  RandyMcMillan:
    ACK e1604b3

Tree-SHA512: fd5a7c5e9a1cbbf0fbb13b5c30b87853c84751da7f0fad08151bda07f1933872ab51cad29a0c0a70ced48e60df6d83bff3f84c2f77d00d22723fae9a8c3534fc
@fanquake @knst
Merge bitcoin#21126: ci: Properly bump to focal for win cross build
4abb768 
fa272df ci: Properly bump to focal for win cross build (MarcoFalke)

Pull request description:

  Fixes bitcoin#21122

ACKs for top commit:
  fanquake:
    ACK fa272df - Looks good to me. I can see the wine version output, `wine-5.0 (Ubuntu 5.0-3ubuntu1)`, in the [log](https://cirrus-ci.com/task/5743559502462976), and `make check` is running.

Tree-SHA512: cd37462afc5512e00cef5e9e7fd1bb5c43c600e833b30cdea2c1c443dc7b0e68f5f2cbaf9d7b655892059af0a226478211db1dc97c10fe4dcdfe666a784f3afc
@fanquake @knst
Merge bitcoin#21138: ci: Re-run wine tests once if they fail
88a45d4 
faa8afe ci: Re-run wine tests once if they fail (MarcoFalke)

Pull request description:

  Works around the intermittent wine issue: bitcoin#21122 (comment)

ACKs for top commit:
  fanquake:
    ACK faa8afe - thanks for following up with this.

Tree-SHA512: cb377a8e62c7fcf38fb5bdd6203af82c6c302c86f7fc219cb756eb7b6d3d4fe4ebf30a36ebba739199ff7814b26256a3701cf38dc5e681b18c2f249cdf6d852e
@laanwj @knst
Merge bitcoin#19522: build: fix building libconsensus with reduced ex…
318c726 
…ports for Darwin targets

de4238f build: consolidate reduced export checks (fanquake)
012bdec build: add building libconsensus to end-of-configure output (fanquake)
8f360e3 build: remove ax_gcc_func_attribute macro (fanquake)
f054a08 build: remove AX_GCC_FUNC_ATTRIBUTE test for dllimport (fanquake)
7cd0a69 build: test for __declspec(dllexport) in configure (fanquake)
1624e17 build: remove duplicate visibility attribute detection (fanquake)

Pull request description:

  Darwin targets do not have a `protected` visibility function attribute, see [LLVM explanation](https://github.com/llvm/llvm-project/blob/8e9a505139fbef7d2e6e9d0adfe1efc87326f9ef/clang/lib/Basic/Targets/OSTargets.h#L131). This means that the `AX_GCC_FUNC_ATTRIBUTE` check for `visibility` fails:
  ```bash
  configure:24513: checking for __attribute__((visibility))
  configure:24537: g++ -std=c++11 -o conftest -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0  -Wl,-headerpad_max_install_names conftest.cpp  >&5
  conftest.cpp:35:56: warning: target does not support 'protected' visibility; using 'default' [-Wunsupported-visibility]
                      int foo_pro( void ) __attribute__((visibility("protected")));
                                                         ^
  1 warning generated.
  configure:24537: $? = 0
  configure:24550: result: no
  ```

  This leads to `EXPORT_SYMBOL` being [defined to nothing](https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/src/script/bitcoinconsensus.h#L29), as `HAVE_FUNC_ATTRIBUTE_VISIBILITY` is not defined, and when building with reduced exports, you end up with a libbitcoinconsensus.dylib that doesn't export any  `_bitcoinconsensus_*` symbols.
  ```bash
  ➜  git:(master) nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
  ➜  git:(master)
  ```

  We do have a [second check](https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/configure.ac#L882) for the `visibility` attribute, which works for Darwin as it's only testing for default visibility, however the result of this check isn't used at all. It was added in dashpay#4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in dashpay#5235, it used the results of the added `AX_GCC_FUNC_ATTRIBUTE` calls.

  This PR removes our usage of the AX_GCC_FUNC_ATTRIBUTE macro entirely, in favour of our own checks in configure. This meant adding a check for `dllexport`, which I've tested as working with both [GCC](https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Function-Attributes.html) and [Clang](https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#dllexport) when building for Windows. I haven't added an equivalent check for `dllimport`, as we weren't actually using the result of that check, we're just testing that `MSC_VER` was defined before using.

  With these changes building a libbitcoinconsensus with reduced exports, when targeting Darwin, works as expected:
  ```bash
  ./autogen.sh
  ./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes --enable-reduce-exports
  make -j8
  ...
  nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
  000000000000a340 T _bitcoinconsensus_verify_script
  00000000000097e0 T _bitcoinconsensus_verify_script_with_amount
  000000000000a3c0 T _bitcoinconsensus_version
  ```

  ```python
  >>> import ctypes
  >>> consensus = ctypes.CDLL("src/.libs/libbitcoinconsensus.dylib")
  >>> print(consensus.bitcoinconsensus_version())
  1
  >>> exit()
  ```

  TODO: Modify a CI job to compile with --enable-reduce-exports and check for symbols in shared lib?

ACKs for top commit:
  laanwj:
    Code review ACK de4238f

Tree-SHA512: d148f3c55d14dac6e9e5b718cc65bb557bcf6f663218d24bc9044b86281bd5dd3d931ebea79c336a58e8ed50d683218c0a9e75494f2267b91097665043e252ae
@knst
Merge bitcoin#21221: [tools] Allow argument/parameter bin packing in …
6bcc86a 
…clang-format

876ac3f [tools] Allow argument/parameter bin packing in clang-format (John Newbery)

Pull request description:

  clang-format documentation for BinPackArguments:

  If `false`, a function call’s arguments will either be all on the same line or will have one line each.

  ```
  true:
  void f() {
    f(aaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaa,
      aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
  }

  false:
  void f() {
    f(aaaaaaaaaaaaaaaaaaaa,
      aaaaaaaaaaaaaaaaaaaa,
      aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
  }
  ```

  https://clang.llvm.org/docs/ClangFormatStyleOptions.html#configurable-format-style-options

  There's no reason to forbid this format. Having multiple arguments or parameters per line can be just as readable as having one per line (and is certainly more readable than having extremely long lines).

ACKs for top commit:
  laanwj:
    ACK 876ac3f
  MarcoFalke:
    review ACK 876ac3f
  vasild:
    ACK 876ac3f

Tree-SHA512: 7c401b4551b458c83dd70883860788b4a60e08a5399171fef27a2f5fdc6b933f6454fe0d396c32d826e3ab537791329da3275ae9b5e9ad36630a6dc2c167e88f
@fanquake @knst
Merge bitcoin#21354: build, doc: Drop no longer required packages fro…
a370253 
…m macOS cross-compiling dependencies

f7f3829 build, doc: Drop libbz2-dev from macOS cross-compiling dependencies (Hennadii Stepanov)
d823936 build, doc: Drop libcap-dev from macOS cross-compiling dependencies (Hennadii Stepanov)

Pull request description:

  The `libcap-dev` and `libbz2-dev` packages are no longer required when cross-compiling for macOS.

ACKs for top commit:
  fanquake:
    ACK f7f3829

Tree-SHA512: 820cdc2724f3346c0942d4d4115fc7206f7bf02889d9fa6cbdbd1d9e3afa03a067c1c3fa64dff596aefdc74898178b7c7d64027a6501486e3b606f4760de04ae
@fanquake @knst
Merge bitcoin#21542: ci: Bump macOS VM image to the latest version
e866b43 
b8e7647 ci: Bump macOS VM image to the latest version (Hennadii Stepanov)

Pull request description:

  On Cirrus CI bump macOS VM from Catalina to Big Sur.

ACKs for top commit:
  fanquake:
    ACK b8e7647 - I'm always going to question bumping things for the sake of it/when there's no reasoning given. In this case, moving from building on macOS 10.x to 11.x shouldn't really lose us any coverage, and may turn up potential macOS quirks a bit earlier.

Tree-SHA512: ab2daee194683ab0553328020fd2fcb918160f466cd380c542e1a9b44f5bea3664fb40b032f1b611ee0107b0efbe278230e067316e2373c3cb0470b205dd2f9d
@knst
Merge bitcoin#20993: test: store subversion (user agent) as string in…
085120d 
… msg_version

de85af5 test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)

Pull request description:

  It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.

  Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing bitcoin#19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.

  So without this patch, we get:

  ```
  $ jq . out.json | grep -m5 strSubVer
        "strSubVer": "2f5361746f7368693a32312e39392e302f"
        "strSubVer": "2f5361746f7368693a302e32302e312f"
        "strSubVer": "2f5361746f7368693a32312e39392e302f"
        "strSubVer": "2f5361746f7368693a302e32302e312f"
        "strSubVer": "2f5361746f7368693a32312e39392e302f"
  ```

  After this patch:

  ```
  $ jq . out2.json | grep -m5 strSubVer
        "strSubVer": "/Satoshi:21.99.0/"
        "strSubVer": "/Satoshi:0.20.1/"
        "strSubVer": "/Satoshi:21.99.0/"
        "strSubVer": "/Satoshi:0.20.1/"
        "strSubVer": "/Satoshi:21.99.0/"
  ```

ACKs for top commit:
  jnewbery:
    utACK de85af5

Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
@knst
fix: adjust functional tests due to dash's support of thread name aft…
43a94f0 
…er v0.12
@laanwj @knst
Merge bitcoin#19809: log: Prefix log messages with function name and …
4774e1e 
…source code location if -logsourcelocations is set

b4511e2 log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)

Pull request description:

  Prefix log messages with function name if `-logfunctionnames` is set.

  Yes, exactly like `-logthreadnames` but for function names instead of thread names :)

  This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.

  For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)

  Without any logging parameters:

  ```
  $ src/bitcoind -regtest
  2020-08-25T03:29:04Z Using RdRand as an additional entropy source
  2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
  2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
  2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
  2020-08-25T03:29:04Z block tree size = 1
  2020-08-25T03:29:04Z nBestHeight = 0
  2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
  2020-08-25T03:29:04Z 0 addresses found from DNS seeds
  ```

  With `-logthreadnames` and `-logfunctionnames`:

  ```
  $ src/bitcoind -regtest -logthreadnames -logfunctionnames
  2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
  2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
  2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
  2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
  2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
  2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
  2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
  2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
  ```

ACKs for top commit:
  laanwj:
    Code review ACK b4511e2
  MarcoFalke:
    review ACK b4511e2 🌃

Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
@knst
fix: format string in llmq/commitment - mismatched arguments
005a6b1
UdjinM6
UdjinM6 approved these changes Apr 10, 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.

rebase looks clean, utACK

PastaPastaPasta
PastaPastaPasta approved these changes Apr 12, 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 005a6b1

@PastaPastaPasta PastaPastaPasta merged commit 54ea926 into dashpay:develop Apr 12, 2024
4 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@knst @UdjinM6 @PastaPastaPasta @fanquake @laanwj