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#21713, #21856, #22061, #22122, #22172, #22261, #22381, #22445, #22447 #6284

Merged
merged 9 commits into from
Sep 27, 2024
Merged

backport: bitcoin#21713, #21856, #22061, #22122, #22172, #22261, #22381, #22445, #22447 #6284

merged 9 commits into from
Sep 27, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Sep 24, 2024 •
edited
Loading

Issue being fixed or feature implemented

Regular backports from bitcoin v22

What was done?

See commits

How Has This Been Tested?

Run unit and functional tests

Breaking Changes

N/A

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.2 milestone Sep 24, 2024
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

fanquake and others added 3 commits September 25, 2024 20:58
@fanquake @knst
Merge bitcoin#22061: ci: Bump multiprocess memory
9ef68d1 
fa0bfc5 ci: Bump multiprocess memory (MarcoFalke)

Pull request description:

  Fixes bitcoin#22059

ACKs for top commit:
  ryanofsky:
    Code review ACK fa0bfc5. Thanks for the update, and interesting to know about bitcoin#21869. It looks like relevant build https://cirrus-ci.com/task/4807455453478912 is succeeding too
  fanquake:
    ACK fa0bfc5

Tree-SHA512: f6e49aadf33199ffa7960c8da0b81bdc5ffea61f373e1b0367d000cdbd214614374b9f1a8b3ce9b8270e6d13a24a2029ab07bddb48e44c86dcb687d645e5ef34
@dongcarl @knst
Merge bitcoin#22381: guix: Test security-check sanity before performi…
b609514 
…ng them

BACKPORT NOTICE: it includes missing commit: d6ef354

lint: Run mypy with --show-error-codes

When using mypy ignore directives, the error code needs to be specified.
Somehow mypy doesn't print it by default...
@fanquake @knst
Merge bitcoin#22447: test: whitelist rpc_rawtransaction peers to spee…
f0c62d5 
…d up tests

a3d6ec5 test: move rpc_rawtransaction tests to < 30s group (Jon Atack)
5a1ed96 test: whitelist rpc_rawtransaction peers to speed up tests (Jon Atack)

Pull request description:

  Speed up the somewhat slow `rpc_rawtransaction.py` test by more than 3x (from 45-55 seconds to 15 seconds on a laptop running 2 x 2.5GHz).

ACKs for top commit:
  mjdietzx:
    ACK a3d6ec5
  kristapsk:
    ACK a3d6ec5
  theStack:
    ACK a3d6ec5 🐎
  brunoerg:
    tACK a3d6ec5

Tree-SHA512: f1d105594c9b5b257a7096b631a6fa5aeb50e330a351f75c2d6ffa7dd73abdb6e1f596a78c16d204a9bac3fe506e0519f9ad96bb8477ab6424c8e18125ccb659
@knst knst marked this pull request as draft September 25, 2024 13:58
MarcoFalke and others added 6 commits September 25, 2024 22:53
@knst
Merge bitcoin#22445: fuzz: Move implementations of non-template fuzz …
1430897 
…helpers from util.h to util.cpp

a2aca20 Move implementations of non-template fuzz helpers (Sriram)

Pull request description:

  There are 78 cpp files that include `util.h` (`grep -iIr "#include <test/fuzz/util.h>" src/test/fuzz | wc -l`). Modifying the implementation of a fuzz helper in `src/test/fuzz/util.h` will cause all fuzz tests to be recompiled. Keeping the declarations of these non-template fuzz helpers in `util.h` and moving their implementations to `util.cpp` will skip the redundant recompilation of all the fuzz tests, and builds these helpers only once in `util.cpp`.

  Functions moved from `util.h` to `util.cpp`:
  - `ConsumeTxMemPoolEntry`
  - `ContainsSpentInput`
  - `ConsumeNetAddr`
  - Methods of `FuzzedFileProvider::(open, read, write, seek, close)`

ACKs for top commit:
  MarcoFalke:
    review ACK a2aca20 🍂

Tree-SHA512: e7037ebb86d0fc56048e4f3d8733eefc21da11683b09d2b22926bda410719628d89c52ddd9b4c18aa243607a66fdb4d13a63e62ca010e66b3ec9174fd18107f0
@fanquake @knst
Merge bitcoin#22261: [p2p/mempool] Two small fixes to node broadcast …
facf685 
…logic

5a77abd [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)

Pull request description:

  1. Only add a transaction to the unbroadcast set when it's added to the mempool

      Currently, if BroadcastTransaction() is called to rebroadcast a
      transaction (e.g. by ResendWalletTransactions()), then we add the
      transaction to the unbroadcast set. That transaction has already been
      broadcast in the past, so peers are unlikely to request it again,
      meaning RemoveUnbroadcastTx() won't be called and it won't be removed
      from m_unbroadcast_txids.

      Net processing will therefore continue to attempt rebroadcast for the
      transaction every 10-15 minutes. This will most likely continue until
      the node connects to a new peer which hasn't yet seen the transaction
      (or perhaps indefinitely).

      Fix by only adding the transaction to the broadcast set when it's added to the mempool.

  2. Allow rebroadcast for same-txid-different-wtxid transactions

      There is some slightly unexpected behaviour when:

      - there is already transaction in the mempool (the "mempool tx")
      - BroadcastTransaction() is called for a transaction with the same txid
        as the mempool transaction but a different witness (the "new tx")

      Prior to this commit, if BroadcastTransaction() is called with
      relay=true, then it'll call RelayTransaction() using the txid/wtxid of
      the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
      in SendMessages(), the wtxid of the new tx will be taken from
      setInventoryTxToSend, but will then be filtered out from the vector of
      wtxids to announce, since m_mempool.info() won't find the transaction
      (the mempool contains the mempool tx, which has a different wtxid from
      the new tx).

      Fix this by calling RelayTransaction() with the wtxid of the mempool
      transaction in this case.

  The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.

ACKs for top commit:
  duncandean:
    reACK 5a77abd
  naumenkogs:
    ACK 5a77abd
  theStack:
    re-ACK 5a77abd
  lsilva01:
    re-ACK bitcoin@5a77abd

Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516
@knst
Merge bitcoin#21856: doc: add OSS-Fuzz section to fuzzing.md doc
c872556 
47c3ea0 doc: add OSS-Fuzz section to fuzzing.md doc (Adam Jonas)

Pull request description:

  This adds documentation about [Bitcoin Core's participation](https://github.com/google/oss-fuzz/pull/5699/files) in Google's OSS-Fuzz program and adds the caveat that the project may not disclose vulnerabilities within the 90-day window described in the [program's disclosure guidelines](https://google.github.io/oss-fuzz/getting-started/bug-disclosure-guidelines/).

ACKs for top commit:
  jonatack:
    ACK 47c3ea0

Tree-SHA512: 87bf0146fb74d1e4b3b8839e6c8f3d53046008a6d5b926ffe5b95be3c396a5e47e47967533422f60b04c4446482f49d210ada410b742f69781a7afde623d704d
@fanquake @knst
Merge bitcoin#22106: refactor: address ProcessNewBlock comments from b…
3b05a99 
…itcoin#21713

e12f287 net: cleanup newly added PeerManagerImpl::ProcessNewBlock (fanquake)
610151f validation: change ProcessNewBlock() to take a CBlock reference (fanquake)

Pull request description:

  Addresses some [post-merge comments](bitcoin#21713 (review)) from bitcoin#21713. Also makes `ChainstateManager::ProcessNewBlock` take a const reference argument, as it [was asked](bitcoin#21713 (comment)) why it was not the case in that PR.

ACKs for top commit:
  jnewbery:
    Code review ACK e12f287
  MarcoFalke:
    review ACK e12f287 🚚

Tree-SHA512: 9c3e7353240c862d50bce2a0f58741c109dd628040b56ed46250103f8ebe9009238b131da710486791e28e3a83c985057b7be0a32aed1a929269b43097c7425b
@knst
Merge bitcoin#22122: ci: Bump macOS image to big-sur-xcode-12.5
9b22501 
faa8dfd ci: Bump macOS image to big-sur-xcode-12.5 (MarcoFalke)

Pull request description:

  Closes bitcoin#22068

ACKs for top commit:
  hebasto:
    ACK faa8dfd, I have reviewed the code and it looks OK, I agree it can be merged, and the Cirrus CI is green.

Tree-SHA512: e29f6290163f3727f3603a3d6b4cf47677f6b02fff370e8d9073962a42bd7ab1ae8d247306e4c41bcadf0a208784344a6229627fe1a883b1e5112df30ea88635
@laanwj @knst
Merge bitcoin#22172: doc: update tor.md, release notes with removal o…
8f06ac9 
…f tor v2 support

2ad034a doc: update release notes with removal of tor v2 support (Jon Atack)
49938ee doc: update tor.md with removal of tor v2 support (Jon Atack)

Pull request description:

  Follow-up documentation to bitcoin#22050 that removed support for Tor version 2 hidden services from Bitcoin Core.

ACKs for top commit:
  laanwj:
    ACK 2ad034a

Tree-SHA512: 0f13f9d1db7e11f1e3d9967b6d17b8dc3144b3ab3a258c706464c5e6ac5cbcf2ce2db4ea54be9939f05a82ebd1e7f325f50b435f9822c08b4f21ed4ac58de0af
@knst knst marked this pull request as ready for review September 25, 2024 17:52
UdjinM6
UdjinM6 approved these changes Sep 25, 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 8f06ac9

PastaPastaPasta
PastaPastaPasta approved these changes Sep 27, 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 8f06ac9

@PastaPastaPasta PastaPastaPasta merged commit bd27f65 into dashpay:develop Sep 27, 2024
12 of 28 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.2
Development

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