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#17938, #19083, #19177, #19233, #19260, #19276, #19282, #19403, #19252, bitcoin-core/gui#3, #6 #5808

Merged
merged 12 commits into from
Jan 16, 2024
Merged

backport: bitcoin#17938, #19083, #19177, #19233, #19260, #19276, #19282, #19403, #19252, bitcoin-core/gui#3, #6 #5808

merged 12 commits into from
Jan 16, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jan 9, 2024 •
edited
Loading

Issue being fixed or feature implemented

Just regular bitcoin backports, mostly from v21

What was done?

How Has This Been Tested?

Run unit/functional tests

Note for reviewers:

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 20.1 milestone Jan 9, 2024
PastaPastaPasta
PastaPastaPasta requested changes Jan 10, 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.

LGTM; but tests fail. Please fix :)

@knst knst marked this pull request as draft January 10, 2024 07:11
@knst knst changed the title backport: bitcoin#17938, #19083, #19177, #19233, #19260, #19276, #19282, #19403, bitcoin-core/gui#3, #6 backport: bitcoin#17938, #19083, #19177, #19233, #19260, #19276, #19282, #19403, #19252 bitcoin-core/gui#3, #6 Jan 10, 2024
@knst knst marked this pull request as ready for review January 10, 2024 19:25
PastaPastaPasta
PastaPastaPasta previously approved these changes Jan 10, 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.

force pushes seem like correct diffs to fix issues

utACK for merge via merge commit

@knst
Copy link
Collaborator Author

knst commented Jan 10, 2024

force push to fix test: https://github.com/dashpay/dash/compare/c7372a970caa4aceb225831bcacf8e2891ed01b0..c4f9250f75b19ac38a512d78af207811397e8031
(and make one of backport partial, 2nd force-push is trivial)

UdjinM6
UdjinM6 requested changes Jan 14, 2024
View reviewed changes
test/functional/p2p_invalid_messages.py Show resolved Hide resolved
test/functional/p2p_filter.py Show resolved Hide resolved
@knst knst requested a review from UdjinM6 January 15, 2024 14:25
PastaPastaPasta
PastaPastaPasta previously approved these changes Jan 15, 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.

re-utACK

UdjinM6
UdjinM6 requested changes Jan 16, 2024
View reviewed changes
test/functional/p2p_filter.py Outdated Show resolved Hide resolved
@knst knst changed the title backport: bitcoin#17938, #19083, #19177, #19233, #19260, #19276, #19282, #19403, #19252 bitcoin-core/gui#3, #6 backport: bitcoin#17938, #19083, #19177, #19233, #19260, #19276, #19282, #19403, #19252, bitcoin-core/gui#3, #6 Jan 16, 2024
@knst knst requested a review from UdjinM6 January 16, 2024 17:54
@knst knst marked this pull request as draft January 16, 2024 17:54
@knst knst marked this pull request as ready for review January 16, 2024 20:10
@knst knst requested a review from PastaPastaPasta January 16, 2024 20:14
UdjinM6
UdjinM6 approved these changes Jan 16, 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

PastaPastaPasta
PastaPastaPasta approved these changes Jan 16, 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.

re-utACK

MarcoFalke added 4 commits January 16, 2024 15:05
@PastaPastaPasta
Merge bitcoin#19233: Make SetMiscWarning() accept bilingual_str argument
673111c 
d49612f Make SetMiscWarning() accept bilingual_str argument (Hennadii Stepanov)
d1ae7c0 Make GetWarnings() return bilingual_str (Hennadii Stepanov)
38e33aa refactor: Make GetWarnings() bilingual_str aware internally (Hennadii Stepanov)

Pull request description:

  This is one more step for consistent usage of `bilingual_str`.

  No new translation messages are defined.

ACKs for top commit:
  laanwj:
    Code review ACK d49612f
  MarcoFalke:
    ACK d49612f 🌂

Tree-SHA512: 7413cb94a85291209c182845f6873350bb9e9ce940647d416c462a136603832fec8a63d792341bf634f07629767c78bc206d3a318cf10c7e87241c114c2496e9
@PastaPastaPasta
Merge bitcoin#19282: RPC: Rephrase generatetoaddress help, and use PA…
d7ea475 
…CKAGE_NAME

0f8f515 RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME (Luke Dashjr)

Pull request description:

Top commit has no ACKs.

Tree-SHA512: 357c6e0bd1b144213ca6cf0bfd649c7a482c2d6d5e98a254d20c8365d228dc71ae1b78aca4918fdbf065f8894ef82f8a475902d605204275bb99fe77d4b42fae
@PastaPastaPasta
Merge bitcoin#19276: ci: Move travis workarounds to .travis.yml
8342f75 
fa71667 ci: Move travis workarounds to .travis.yml (MarcoFalke)

Pull request description:

  It seems odd to have travis related workarounds in the general ci config files. Fix that oddity by moving the travis related workarounds to the travis yaml file.

  For unexplained reasons, this should also work around and thus close bitcoin#19171

ACKs for top commit:
  hebasto:
    ACK fa71667, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: b4419d38e2b41f6e4d6e6b7658f1d972c40c390a49fe78808f8640d28efd84cc6668ce292d45b7c539e65b9e2ecbad10e796cb8f9329a0f1e7d0132ce962d226
@PastaPastaPasta
Merge bitcoin-core/gui#3: scripted-diff: Make SeparatorStyle a scoped…
89b9ff5 
… enum

25f3554 scripted-diff: Make SeparatorStyle a scoped enum (Hennadii Stepanov)

Pull request description:

  This PR is [split](bitcoin#17877 (comment)) from bitcoin#17877 and makes `BitcoinUnits::SeparatorStyle` a scoped enum.

ACKs for top commit:
  MarcoFalke:
    review ACK 25f3554 🚐

Tree-SHA512: 578f1340a476cf79faa109a83815d3c75e26d9c18873e653d7624b52428ccb2677293116db0a60ae14c949d63b64988fc5a39c7184c2352b87b00e8ddaaaf474
meshcollider and others added 8 commits January 16, 2024 15:05
@meshcollider @PastaPastaPasta
Merge bitcoin#17938: Disallow automatic conversion between disparate …
1e87dc0 
…hash types

4d73691 Disallow automatic conversion between hash types (Ben Woosley)
fa9ef2c Remove an apparently unnecessary conversion (Ben Woosley)
966a22d Explicitly support conversion between equivalent hash types (Ben Woosley)
f32c1e0 Use explicit conversion from WitnessV0KeyHash -> CKeyID (Ben Woosley)
2c54217 Use explicit conversion from PKHash -> CKeyID (Ben Woosley)
a9e451f Convert CPubKey to WitnessV0KeyHash directly (Ben Woosley)
3fcc468 Prefer explicit CScriptID construction (Ben Woosley)
0a5ea32 Prefer explicit uint160 conversion (Ben Woosley)

Pull request description:

  This bases the script/standard hash types, TxDestination-related and CScriptID on a base template which does not silently convert the underlying `uintN` type.

  Inspired by and built on bitcoin#17924. Commits are small and focused to ease review.

  Note some of these changes may be relative to existing bugs of the same sort as bitcoin#17924. See particularly "Convert CPubKey to WitnessV0KeyHash directly" and "Remove an apparently unnecessary conversion".

ACKs for top commit:
  achow101:
    ACK 4d73691
  meshcollider:
    re-utACK 4d73691

Tree-SHA512: f1b3284ddc6fb6c6e726f2c22668b6d732d45eb5418262ed2b9c728f60be7be43dfb414b6ddd9915025c8dcd7f360dc3b46e997a945a2feb95b0e5c4f05d6b54
@PastaPastaPasta
Merge bitcoin-core/gui#6: Do not truncate node flag strings in debugw…
f5cb202 
…indow peers details tab

0ac09c9 qt: Do not truncate node flag strings in debugwindow.ui peers details tab. (saibato)

Pull request description:

  Fix: When fiddling around with new node flags other than the usual.

  I saw that not all possible node flag strings i.e. the UNKNOWN[..] where
  visible in peers details tab.
  Since v18.2 fixed size was set to 300 and sliding is thereby limited.

  A fix on my old linux cruft and small screen was to set minimumSize width to -1 or 0.
  Qt will then autosize the slider to the max string length.

  Thereby i had full display of all flags inclusive sliding without to fullscreen the window.

  Not sure if this is even an issue for those who can afford big screens or high res macs?
  Feedback welcome.

  BTW: nice side effect now again easy to scroll trough long version names of the node.
  can't wait to see strings like /Satoshi:0.23.99/NOX2NOX4NOX32  or what ever fits in the version string.

ACKs for top commit:
  hebasto:
    ACK 0ac09c9, tested on Linux Mint 20 (x86_64, Qt 5.12.8).
  promag:
    Tested ACK 0ac09c9 on macos.

Tree-SHA512: a1601b5e35f10b1fd9407b28142ca00c1b985a822be5d23be4d7d3376211450f06e17f962c44b8b40977f8f8bbbb701cac1c5abb4afb3618da76385dfac848a3
@laanwj @PastaPastaPasta
Merge bitcoin#19403: build: improve __builtin_clz* detection
54501fb 
9952242 build: improve builtin_clz* detection (fanquake)

Pull request description:

  Fixes bitcoin#19402.

  The way we currently test for `__builtin_clz*` support with `AC_CHECK_DECLS` does not work with Clang:
  ```bash
  configure:21492: clang++-10 -std=c++11 -c -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS conftest.cpp >&5
  conftest.cpp:100:10: error: builtin functions must be directly called
    (void) __builtin_clz;
           ^
  1 error generated.
  ```

  This also removes the `__builtin_clz()` check, as we don't actually use it anywhere, and it's trvial to re-add detection if we do start using it at some point. If this is controversial then I'll add a test for it as well.

ACKs for top commit:
  sipa:
    ACK 9952242
  laanwj:
    ACK 9952242

Tree-SHA512: 695abb1a694a01a25aaa483b4fffa7d598842f2ba4fe8630fbed9ce5450b915c33bf34bb16ad16a16b702dd7c91ebf49fe509a2498b9e28254fe0ec5177bbac0
@PastaPastaPasta
Merge bitcoin#19177: test: Fix and clean p2p_invalid_messages functio…
94680f4 
…nal tests

af2a145 Refactor resource exhaustion test (Troy Giorshev)
5c4648d Fix "invalid message size" test (Troy Giorshev)
ff1e7b8 Move size limits to module-global (Troy Giorshev)
57890ab Remove two unneeded tests (Troy Giorshev)

Pull request description:

  This PR touches only the p2p_invalid_messages.py functional test module.  There are two main goals accomplished here.  First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons.  Second, it refactors the file into a single consistent style.  This file appears to have originally had two authors, with different styles and some test duplication.

  It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](bitcoin#18242) and [AltNet](bitcoin#18989) changes.

  This should probably go in ahead of bitcoin#19107, but the two are not strictly related.

ACKs for top commit:
  jnewbery:
    ACK af2a145
  MarcoFalke:
    re-ACK af2a145 🍦

Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
@UdjinM6 @PastaPastaPasta
fix: bitcoin#18808 follow-up
8465730 
The bug was introduced in v19 via 5118
@PastaPastaPasta
Merge bitcoin#19083: test: msg_mempool, fRelay, and other bloomfilter…
cc6ebdb 
… tests

(BACKPORT NOTICE: p2p_filter.py has also fixes for d5fbd4a:test/functional/p2p_filter.py)
+    Merge bitcoin#18726: test: check misbehavior more independently in p2p_filter.py
---------------------

dca7394 scripted-diff: rename node to peer for mininodes (gzhao408)
0474ea2 [test] fix race conditions and test in p2p_filter (gzhao408)
4ef80f0 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408)
497a619 [test] add BIP 37 test for node with fRelay=false (gzhao408)
e8acc60 [test] add mempool msg test for node with bloomfilter enabled (gzhao408)

Pull request description:

  This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_:
  1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)).
  2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`.
  3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled.
  4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py.
  5. Fixes race conditions in p2p_filter.py

ACKs for top commit:
  MarcoFalke:
    ACK dca7394 only changes is restoring accidentally deleted test 🍮
  jonatack:
    ACK dca7394 modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to.

Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3
@fanquake @PastaPastaPasta
Merge bitcoin#19260: p2p: disconnect peers that send filterclear + up…
65217a7 
…date existing filter msg disconnect logic

3a10d93 [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408)
ff8c430 [test] test disconnect for filterclear (gzhao408)
1c6b787 [netprocessing] disconnect node that sends filterclear (gzhao408)

Pull request description:

  Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages.

  Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](https://github.com/bitcoin/bitcoin/blob/19e919217e6d62e3640525e4149de1a4ae04e74f/src/net_processing.cpp#L2218), but bitcoin#8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See bitcoin#19204).

  Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them.

ACKs for top commit:
  jnewbery:
    Code review ACK 3a10d93
  naumenkogs:
    utACK 3a10d93
  gillichu:
    tested ACK: quick test_runner on macOS [`3a10d93`](bitcoin@3a10d93)
  MarcoFalke:
    re-ACK 3a10d93 only change is replacing false with true 🚝

Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
@PastaPastaPasta
Merge bitcoin#19252: test: wait for disconnect in disconnect_p2ps + b…
6f02bab 
…loomfilter test followups

9a40cfc [refactor] use waiting inside disconnect_p2ps (gzhao408)
aeb9fb4 [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408)
e81942d [test] logging and style followups for bloomfilter tests (gzhao408)

Pull request description:

  Followup to bitcoin#19083 which adds bloomfilter-related tests.

  1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](bitcoin#19083 (comment)). And clean up any redundant `wait_until`s in the functional tests.
  2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](bitcoin#19083 (review))

ACKs for top commit:
  jonatack:
    Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc`
  MarcoFalke:
    ACK 9a40cfc 🐂

Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87
@PastaPastaPasta PastaPastaPasta merged commit dd7fac4 into dashpay:develop Jan 16, 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
@knst @UdjinM6 @PastaPastaPasta @meshcollider @laanwj @fanquake