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#24946, #22649, #22704, #23806, #26153 #5702

Merged
merged 5 commits into from
Nov 19, 2023
Merged

backport: Merge bitcoin#24946, #22649, #22704, #23806, #26153 #5702

merged 5 commits into from
Nov 19, 2023

Conversation

ogabrielides
Copy link
Collaborator

@ogabrielides ogabrielides commented Nov 15, 2023 •
edited
Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

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 (for repository code-owners and collaborators only)

@ogabrielides ogabrielides force-pushed the bitcoin_backport_24946 branch from 7fea337 to 186416f Compare November 15, 2023 17:23
knst
knst previously approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

test/sanitizer_suppressions/ubsan Outdated Show resolved Hide resolved
@ogabrielides ogabrielides added this to the 20.1 milestone Nov 16, 2023
@UdjinM6
Copy link

UdjinM6 commented Nov 16, 2023 •
edited
Loading

26153: missing changes in src/test/fuzz/crypto_diff_fuzz_chacha20.cpp - should be partial. Also needs rebase.

@ogabrielides
Copy link
Collaborator Author

ogabrielides commented Nov 16, 2023 •
edited
Loading

26153: missing changes in src/test/fuzz/crypto_diff_fuzz_chacha20.cpp - should be partial. Also needs rebase.

This file wasn't there. Unless if it was backported in the meanwhile (and that's why the PR needs rebase)

@UdjinM6
Copy link

UdjinM6 commented Nov 16, 2023 •
edited
Loading

We don't have that file yet, so backport should be marked partial. And I asked for a rebase cause you'd have to change the name of the commit anyway and we need to fix Merge Fast-Forward Only check here.

@UdjinM6
Copy link

UdjinM6 commented Nov 16, 2023

or you can only change commit name and I'll rebase it from GH GUI afterwards

@UdjinM6
Copy link

UdjinM6 commented Nov 16, 2023

or even better - backport 22704 and 23806 (as discussed in Slack, should be trivial) and add missing changes to 26153

@ogabrielides ogabrielides force-pushed the bitcoin_backport_24946 branch from 186416f to 2e2bcfb Compare November 17, 2023 09:59
@ogabrielides ogabrielides changed the title backport: Merge bitcoin#24946, #26153 backport: Merge bitcoin#24946, #22649, #22704, #23806, #26153 Nov 17, 2023
@ogabrielides ogabrielides force-pushed the bitcoin_backport_24946 branch from 2e2bcfb to 6183bb1 Compare November 17, 2023 12:10
@ogabrielides ogabrielides requested a review from knst November 17, 2023 12:12
knst
knst approved these changes Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

urACK

@UdjinM6 UdjinM6 force-pushed the bitcoin_backport_24946 branch from 6183bb1 to 7f81bf0 Compare November 17, 2023 12:39
@UdjinM6
Copy link

UdjinM6 commented Nov 17, 2023

rebased from GH GUI to fix Merge Fast-Forward Only check

UdjinM6
UdjinM6 approved these changes Nov 17, 2023
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

PastaPastaPasta
PastaPastaPasta approved these changes Nov 19, 2023
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 merging via merge commit

MacroFake and others added 5 commits November 19, 2023 10:20
@PastaPastaPasta
Merge bitcoin#24946: Unroll the ChaCha20 inner loop for performance
e98e5dc 
81c09ee Unroll the ChaCha20 inner loop for performance (Pieter Wuille)

Pull request description:

  Unrolling the inner ChaCha20 loop gives a ~15% speedup for me in the CHACHA20_* benchmarks. It's a simple change, this performance helps with RNG generation, and will matter more for BIP324.

ACKs for top commit:
  martinus:
    tested ACK  81c09ee with clang++ 13.0.1, test `CHACHA20_1MB`:
  MarcoFalke:
    ACK 81c09ee 🍟

Tree-SHA512: 108bd0ba573bb08de92d611e7be7c09a2c2700f9655f44129b87f9b71f7e101dfc6bd345783e7b4b9b40f0b003913cf59187f422da8cdb5b20887f7855b2611a
@PastaPastaPasta
Merge bitcoin#22649: fuzz: Avoid OOM in system fuzz target
47828bd 
fa77183 fuzz: Avoid OOM in system fuzz target (MarcoFalke)

Pull request description:

  If the inputs size is unlimited, the target may consume unlimited memory, because the argsmanager stores the argument names. Limiting the size should fix this issue.

  Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36906

ACKs for top commit:
  practicalswift:
    cr ACK fa77183

Tree-SHA512: 6edfcf324ee9d94e511038ee01340f02db50bcb233af3f1a1717c3602164c88528d9d987e971ec32f1a4593b868019bea0102c53c9b02bfefec3dfde959483cf
@laanwj @PastaPastaPasta
Merge bitcoin#22704: fuzz: Differential fuzzing to compare Bitcoin Co…
c8650ec 
…re's and D. J. Bernstein's implementation of ChaCha20

4d0ac72 [fuzz] Add fuzzing harness to compare both implementations of ChaCha20 (stratospher)
65ef932 [fuzz] Add D. J. Bernstein's implementation of ChaCha20 (stratospher)

Pull request description:

  This PR compares Bitcoin Core's implementation of ChaCha20 with D. J. Bernstein's in order to find implementation discrepancies if any.

ACKs for top commit:
  laanwj:
    Code review ACK 4d0ac72

Tree-SHA512: f826144b4db61b9cbdd7efaaca8fa9cbb899953065bc8a26820a566303b2ab6a17431e7c114635789f0a63fbe3b65cb0bf2ab85baf882803a5ee172af4881544
@PastaPastaPasta
Merge bitcoin#23806: fuzz: follow up for bitcoin#22704
1915914 
8f79831 Refactor the chacha20 differential fuzz test (stratospher)

Pull request description:

  This PR addresses [comments from bitcoin#22704](https://github.com/bitcoin/bitcoin/pull/22704/files#discussion_r771510963)  to make the following changes in `src/test/fuzz/crypto_diff_fuzz_chacha20.cpp`:

  - replace `memcmp()` with ==
  - add a missing assert statement to compare the encrypted bytes

Top commit has no ACKs.

Tree-SHA512: 02338460fb3a89e732558bf00f3aebf8f04daba194e03ae0e3339bb2ff6ba35d06841452585b739047a29f8ec64f36b1b4ce2dfa39a08f6ad44a6a937e7b3acb
@fanquake @PastaPastaPasta
Merge bitcoin#26153: Reduce wasted pseudorandom bytes in ChaCha20 + v…
5a4406e 
…arious improvements

511aa4f Add unit test for ChaCha20's new caching (Pieter Wuille)
fb243d2 Improve test vectors for ChaCha20 (Pieter Wuille)
93aee8b Inline ChaCha20 32-byte specific constants (Pieter Wuille)
62ec713 Only support 32-byte keys in ChaCha20{,Aligned} (Pieter Wuille)
f21994a Use ChaCha20Aligned in MuHash3072 code (Pieter Wuille)
5d16f75 Use ChaCha20 caching in FastRandomContext (Pieter Wuille)
38eaece Add fuzz test for testing that ChaCha20 works as a stream (Pieter Wuille)
5f05b27 Add xoroshiro128++ PRNG (Martin Leitner-Ankerl)
12ff724 Make unrestricted ChaCha20 cipher not waste keystream bytes (Pieter Wuille)
6babf40 Rename ChaCha20::Seek -> Seek64 to clarify multiple of 64 (Pieter Wuille)
e37bcaa Split ChaCha20 into aligned/unaligned variants (Pieter Wuille)

Pull request description:

  This is an alternative to bitcoin#25354 (by my benchmarking, somewhat faster), subsumes bitcoin#25712, and adds additional test vectors.

  It separates the multiple-of-64-bytes-only "core" logic (which becomes simpler) from a layer around which performs caching/slicing to support arbitrary byte amounts. Both have their uses (in particular, the MuHash3072 code can benefit from multiple-of-64-bytes assumptions), plus the separation results in more readable code. Also, since FastRandomContext effectively had its own (more naive) caching on top of ChaCha20, that can be dropped in favor of ChaCha20's new built-in caching.

  I thought about rebasing bitcoin#25712 on top of this, but the changes before are fairly extensive, so redid it instead.

ACKs for top commit:
  ajtowns:
    ut reACK 511aa4f
  dhruv:
    tACK crACK 511aa4f

Tree-SHA512: 3aa80971322a93e780c75a8d35bd39da3a9ea570fbae4491eaf0c45242f5f670a24a592c50ad870d5fd09b9f88ec06e274e8aa3cefd9561d623c63f7198cf2c7
@PastaPastaPasta PastaPastaPasta merged commit 722d14c into dashpay:develop Nov 19, 2023
6 of 10 checks passed
@ogabrielides ogabrielides deleted the bitcoin_backport_24946 branch November 19, 2023 16:29
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

@knst knst knst 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
@ogabrielides @UdjinM6 @knst @PastaPastaPasta @laanwj @fanquake