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: Merge bitcoin#19370, 19562, 19538, 19272, 19214, 19429, 18990, 20927, 20507 #5770

Merged
merged 8 commits into from
Feb 7, 2024
Merged

backport: Merge bitcoin#19370, 19562, 19538, 19272, 19214, 19429, 18990, 20927, 20507 #5770

merged 8 commits into from
Feb 7, 2024

Conversation

vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Dec 16, 2023 •
edited
Loading

Bitcoin Backports

@vijaydasmp vijaydasmp changed the title backport: backport: Merge bitcoin#19884, 19473 Dec 16, 2023
@vijaydasmp
Copy link
Author

Hello @UdjinM6 , For some reason, GitLab ci is not triggering

@UdjinM6
Copy link

UdjinM6 commented Dec 19, 2023 •
edited
Loading

Pls rebase. This should hopefully help triggering CI.

Nvm, it already started and failed :)

@vijaydasmp vijaydasmp force-pushed the bp22_26_1 branch 2 times, most recently from 5220591 to 51527e5 Compare December 23, 2023 17:50
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19884, 19473 backport: Merge bitcoin#19322 Dec 23, 2023
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19322 backport: Merge bitcoin#19322,19370, 19562, 19538, 19272 Dec 24, 2023
@vijaydasmp vijaydasmp force-pushed the bp22_26_1 branch 2 times, most recently from 5b69a8a to 1e68b59 Compare December 24, 2023 03:37
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2024

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19322,19370, 19562, 19538, 19272 backport: Merge bitcoin#19322,19370, 19562, 19538, 19272, 19214, 19429, 18990 Jan 13, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp force-pushed the bp22_26_1 branch 5 times, most recently from cc3b5d2 to b528f71 Compare January 21, 2024 14:29
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#19322,19370, 19562, 19538, 19272, 19214, 19429, 18990 backport: Merge bitcoin#19370, 19562, 19538, 19272, 19214, 19429, 18990 Jan 21, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 3, 2024

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp force-pushed the bp22_26_1 branch 2 times, most recently from bfc5bf2 to 4ed07ce Compare February 4, 2024 04:17
@vijaydasmp vijaydasmp marked this pull request as ready for review February 4, 2024 07:13
UdjinM6
UdjinM6 requested changes Feb 4, 2024
View reviewed changes
src/net_processing.cpp Outdated Show resolved Hide resolved
test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved
test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved
test/functional/p2p_invalid_messages.py Show resolved Hide resolved
test/functional/wallet_encryption.py Outdated Show resolved Hide resolved
@vijaydasmp vijaydasmp force-pushed the bp22_26_1 branch 2 times, most recently from dfdbdfc to 265cf95 Compare February 5, 2024 13:14
@vijaydasmp
Copy link
Author

Hello @UdjinM6 apparently unrelated in feature_llmq_simplepose.py", line 203, in repair_masternodes, causing test failure,
https://gitlab.com/dashpay/dash/-/jobs/6096975858

@vijaydasmp vijaydasmp requested a review from UdjinM6 February 6, 2024 03:38
@knst
Copy link
Collaborator

knst commented Feb 6, 2024

Hello @UdjinM6 apparently unrelated in feature_llmq_simplepose.py", line 203,

Please ignore, seems as failure unrelated, see #5859 (comment)

@UdjinM6 UdjinM6 added this to the 20.1 milestone Feb 6, 2024
@vijaydasmp vijaydasmp force-pushed the bp22_26_1 branch 2 times, most recently from 6fbfd3c to f4b26d1 Compare February 6, 2024 15:03
UdjinM6
UdjinM6 approved these changes Feb 6, 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.

utACK

@vijaydasmp
Copy link
Author

Hello @PastaPastaPasta , requesting review

PastaPastaPasta
PastaPastaPasta approved these changes Feb 7, 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 merge via merge commit

laanwj and others added 8 commits February 7, 2024 10:27
@laanwj @PastaPastaPasta
Merge bitcoin#19370: Static asserts for consistency of fee defaults
6d7894c 
1554b54 Static asserts for consistency of fee defaults. (Daniel Kraft)

Pull request description:

  This adds `static_assert`'s that ensure that the default values given for fee levels in the wallet (minimum fee and incremental feerate increase) are at least as high as the corresponding levels configured in the core node policy.  Since the core policy values are enforced by the network, it makes sense for the wallet to be conservative and above (or at least not below) this.

ACKs for top commit:
  laanwj:
    code review ACK 1554b54, these assumptions seem straightforward

Tree-SHA512: 50e5adf082f467062334377f82a3ee75bcfd436afc65bd0eb33c8d0549d6d90fd1f48c31f60cabe523eb59be9efa8ae0879e9e09cd51ca9c1bd466631ce03cf4
@fanquake @PastaPastaPasta
Merge bitcoin#19562: test: Fix fuzzer compilation on macOS
c69474a 
c8992e8 test: Fix fuzzer compilation on macOS fixes bitcoin#19557 (freenancial)

Pull request description:

  fixes bitcoin#19557

  Before the fix:
  ```
  ➜  bitcoin git:(fix-fuzzer-macos) make
  Making all in src
    CXX      test/fuzz/addition_overflow-addition_overflow.o
  In file included from test/fuzz/addition_overflow.cpp:7:
  ./test/fuzz/util.h:335:13: error: no matching function for call to 'AdditionOverflow'
          if (AdditionOverflow((uint64_t)fuzzed_file->m_offset, random_bytes.size())) {
              ^~~~~~~~~~~~~~~~
  ./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned long long' vs. 'unsigned long')
  NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
                 ^
  ./test/fuzz/util.h:346:13: error: no matching function for call to 'AdditionOverflow'
          if (AdditionOverflow(fuzzed_file->m_offset, n)) {
              ^~~~~~~~~~~~~~~~
  ./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long long' vs. 'long')
  NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
                 ^
  ```

  After the fix:
  ```
  ➜  bitcoin git:(fix-fuzzer-macos) ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++ --disable-asm && make clean && make -j5
  ...
  ...
    CXXLD    test/fuzz/uint256_deserialize
  Making all in doc/man
  make[1]: Nothing to be done for `all'.
  make[1]: Nothing to be done for `all-am'.
  ```

ACKs for top commit:
  fanquake:
    ACK c8992e8 - tested that compiling works on macOS.
  MarcoFalke:
    review ACK c8992e8

Tree-SHA512: 965cdc61b30db0e2209c91b29f0d42de927a9a5b85e1e70f22d1452e0955f876726c7a8c1d1a5f448f12bf24eec3000802071cd4ae28d8605343fd43d174ca84
@PastaPastaPasta
Merge bitcoin#19272: net, test: invalid p2p messages and test framewo…
edf1789 
…rk improvements

56010f9 test: hoist p2p values to test framework constants (Jon Atack)
75447f0 test: improve msg sends and p2p disconnections in p2p_invalid_messages (Jon Atack)
5796019 test: refactor test_large_inv() into 3 tests with common method (Jon Atack)
e2b21d8 test: add p2p_invalid_messages logging (Jon Atack)
9fa494d net: update misbehavior logging for oversized messages (Jon Atack)

Pull request description:

  ...seen while reviewing bitcoin#19264, bitcoin#19252, bitcoin#19304 and bitcoin#19107:

  in `net_processing.cpp`
  - make the debug logging for oversized message size misbehavior the same for `addr`, `getdata`, `headers` and `inv` messages

  in `p2p_invalid_messages`
  - add missing logging
  - improve assertions/message sends, move cleanup disconnections outside the assertion scopes
  - split a slowish 3-part test into 3 order-independent tests
  - add a few p2p constants to the test framework

ACKs for top commit:
  troygiorshev:
    reACK 56010f9
  MarcoFalke:
    ACK 56010f9 🎛

Tree-SHA512: db67b70278f8d4c318907e105af54b54eb3afd15500f9aa0c98034f6fd4bd1cf9ad1663037bd9b237ff4890f3059b37291a6498d8d6ae2cc38efb9f045f73310
@laanwj @PastaPastaPasta
Merge bitcoin#19214: Auto-detect SHA256 implementation in benchmarks
0ac06c1 
addf18d Call SHA256AutoDetect in benchmark setup (Pieter Wuille)

Pull request description:

  It seems `SHA256AutoDetect()` was not being called in benchmarks, making the numbers only reflect the naive implementation. Fix this by calling it in bench_bitcoin's setup.

ACKs for top commit:
  fjahr:
    tested ACK addf18d
  pstratem:
    ACK addf18d
  laanwj:
    ACK addf18d

Tree-SHA512: 3ba4b068145942df1429bf5913e3f685511e6ebeae2c1a3f9b8ac0144f6db1c7df456f88f480a2129f3e1602e3bf6a39530bb96e2c74c03ddb19324cec6799c7
@PastaPastaPasta
Merge bitcoin#19429: test: Fix intermittent failure in wallet_encryption
13d7f00 
fabd33b test: Fix intermittent failure in wallet_encryption (MarcoFalke)

Pull request description:

  Iterating all crypted keys might take time.

  E.g.

  ```
   node0 2020-07-01T14:41:19.227367Z [httpworker.0] ThreadRPCServer method=walletpassphrase user=__cookie__
   node0 2020-07-01T14:41:24.377142Z [httpworker.0] queue run of timer lockwallet() in 100000000 seconds (using HTTP)
  ...
   test  2020-07-01T14:41:24.379000Z TestFramework (ERROR): Assertion failed
                                     Traceback (most recent call last):
                                       File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 117, in main
                                         self.run_test()
                                       File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_encryption.py", line 88, in run_test
                                         assert_greater_than(expected_time + 5, actual_time) # 5 second buffer
                                       File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 54, in assert_greater_than
                                         raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
                                     AssertionError: 1693614483 <= 1693614484
  ```

  https://cirrus-ci.com/task/5322429885054976?command=ci#L4517

ACKs for top commit:
  achow101:
    ACK fabd33b

Tree-SHA512: 7a3ccdfc0cdc05fef1f942d3167d100ed63422eb54c05405c884ed91162b7bdb5ce54cb5a981b99a6df2e4af1ea834ccd7d5156531c8c14ea13e735becd6b377
@PastaPastaPasta
Merge bitcoin#18990: log: Properly log txs rejected from mempool
4bcbb8d 
fa9f20b log: Properly log txs rejected from mempool (MarcoFalke)

Pull request description:

  Currently `CheckTxInputs` rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues:

  * A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user.
  * A rejected p2p transaction will log the failure twice. Once with the `MEMPOOLREJ` flag, and once unconditionally.
  * A rejected orphan transaction will log no failure.

  Fix all issues by simply returning the state to the caller, like it is done for all other rejections.

  The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review.

ACKs for top commit:
  naumenkogs:
    utACK fa9f20b
  rajarshimaitra:
    Concept ACK. Compiled and ran tests. `fa9f20b`
  jnewbery:
    code review ACK fa9f20b

Tree-SHA512: 86cc17b2a9239c01c4fc3f254ad48ee1d3883266966b9811030176338b9ac3deaea7ea5babfb8bbf739d7440154e30011fede8f9313175f199d4a062af6494f7
@PastaPastaPasta
Merge bitcoin#20927: [refactor] [net] Clean up InactivityCheck()
8038be1 
bf100f8 [net] Cleanup InactivityChecks() and add commenting about time (John Newbery)
06fa85c [net] InactivityCheck() takes a CNode reference (John Newbery)

Pull request description:

  This is a pure refactor and should not change any behavior. It clarifies and documents the InactivityCheck() function

  This makes bitcoin#20721 easier to review. In particular, this function uses a mixture of (unmockable) system time and mockable time. It's important to understand where those are being used when reviewing bitcoin#20721.

  bitcoin#20721 doesn't require this change, so if others don't agree that it's useful and makes review easier, then I'm happy to close this and just do bitcoin#20721 directly.

ACKs for top commit:
  fanquake:
    ACK bf100f8
  MarcoFalke:
    review ACK bf100f8 💫

Tree-SHA512: 7b001de2a5fbe8a6dc37baeae930db5775290afb2e8a6aecdf13161f1e5b06ef813bc6291d8ee5cefcf1e430c955ea702833a8db84192eebe6e6acf0b9304cb2
@PastaPastaPasta
Merge bitcoin#20507: sync: print proper lock order location when doub…
18500ea 
…le lock is detected

db058ef sync: use HasReason() in double lock tests (Vasil Dimov)
a21dc46 sync: const-qualify the argument of double_lock_detected() (Vasil Dimov)
6d3689f sync: print proper lock order location when double lock is detected (Vasil Dimov)

Pull request description:

  Before:
  ```
  Assertion failed: detected double lock at src/sync.cpp:153, details in debug log.
  ```

  After:
  ```
  Assertion failed: detected double lock for 'm' in src/test/sync_tests.cpp:40 (in thread ''), details in debug log.
  ```

ACKs for top commit:
  jonasschnelli:
    utACK db058ef
  ajtowns:
    ACK db058ef
  hebasto:
    ACK db058ef, tested on Linux Mint 20 (x86_64).

Tree-SHA512: 452ddb9a14e44bb174135b39f2219c76eadbb8a6c0e80d64a25f995780d6dbc7b570d9902616db94dbfabaee197b5828ba3475171a68240ac0958fb203a7acdb
@PastaPastaPasta PastaPastaPasta merged commit 08b22dd into dashpay:develop Feb 7, 2024
4 of 5 checks passed
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
None yet
Projects
None yet
Milestone
20.1
Development

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