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.

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

backport: Merge bitcoin#(partial) 21940,(partial) 22232,22505,22407,22510,22383,13533,22528,22538,22139 #5663

Merged
merged 10 commits into from
Dec 4, 2023

Conversation

vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Nov 1, 2023

Issue being fixed or feature implemented

bitcoin backports

What was done?

Backported bitcoin changes

How Has This Been Tested?

CI Run

@vijaydasmp vijaydasmp changed the title Merge bitcoin/bitcoin#21940: refactor: Mark CAddrMan::Select and GetA… backport: Merge bitcoin#21940 Nov 3, 2023
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#21940 backport: Merge bitcoin#21940,22232,22505,22407,22510,22383,13533 Nov 6, 2023
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#21940,22232,22505,22407,22510,22383,13533 backport: Merge bitcoin#21940,22232,22505,22407,22510,22383,13533,22528,22538,22139 Nov 6, 2023
@vijaydasmp vijaydasmp force-pushed the bp23_2 branch 2 times, most recently from 1013880 to bb0d506 Compare November 7, 2023 02:51
@vijaydasmp vijaydasmp marked this pull request as ready for review November 9, 2023 06:05
@vijaydasmp
Copy link
Author

Hello @UdjinM6 @knst @PastaPastaPasta , please review

src/test/fuzz/addrman.cpp Show resolved Hide resolved
src/consensus/tx_verify.h Show resolved Hide resolved
@vijaydasmp vijaydasmp changed the title backport: Merge bitcoin#21940,22232,22505,22407,22510,22383,13533,22528,22538,22139 backport: Merge bitcoin#(partial) 21940,(partial) 22232,22505,22407,22510,22383,13533,22528,22538,22139 Nov 10, 2023
@vijaydasmp vijaydasmp requested a review from UdjinM6 November 11, 2023 13:23
@UdjinM6 UdjinM6 added this to the 21 milestone Nov 11, 2023
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.

LGTM, utACK

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

1. Uninstall Guix itself according to the way you installed it. (e.g. `sudo apt
purge guix` for Ubuntu packaging, `sudo make uninstall` for
built-from-source).
1. Uninstall Guix itself according to the way you installed it (e.g. `sudo apt
Copy link
Collaborator

@knst knst Nov 12, 2023

Choose a reason for hiding this comment

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

should it be partial due to missing changes in doc/build-openbsd.md (depends on bitcoin#22335)

@UdjinM6
Copy link

UdjinM6 commented Nov 16, 2023

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

@vijaydasmp
Copy link
Author

Hello @PastaPastaPasta

@knst

This comment was marked as outdated.

@UdjinM6
Copy link

UdjinM6 commented Nov 17, 2023

@knst wrong PR :) see #5677 (comment)

@vijaydasmp
Copy link
Author

Hello @UdjinM6 , https://gitlab.com/dashpay/dash/-/jobs/5562361219 has timed out,
requesting re-trigger CI

@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Nov 20, 2023
@vijaydasmp
Copy link
Author

Hello @PastaPastaPasta , requesting review

@vijaydasmp
Copy link
Author

@PastaPastaPasta , requesting review

@vijaydasmp
Copy link
Author

please let me know if there are any further comments on this PR. @PastaPastaPasta @UdjinM6 @knst

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

MarcoFalke and others added 10 commits December 3, 2023 20:13
…tAddr const

fae108c Fix incorrect whitespace in addrman (MarcoFalke)
fa32024 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b fuzz: Actually use const addrman (MarcoFalke)
fae0c79 refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934 refactor: Mark CAddrMan::Select const (MarcoFalke)

Pull request description:

  To clarify that a call to this only changes the random state and nothing else.

ACKs for top commit:
  jnewbery:
    Code review ACK fae108c
  theStack:
    re-ACK fae108c 🍦

Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
…nt32_t instead of signed int

fa621ed refactor: Pass script verify flags as uint32_t (MarcoFalke)

Pull request description:

  The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.

  Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.

  Fixes bitcoin#22233

ACKs for top commit:
  theStack:
    Concept and code review ACK fa621ed
  kristapsk:
    ACK fa621ed
  jonatack:
    ACK fa621ed

Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
…t from Good()

f036dfb [addrman] Remove unused test_before_evict argument from Good() (John Newbery)

Pull request description:

  This has never been used in the public interface method since it was
  introduced in bitcoin#9037.

ACKs for top commit:
  lsilva01:
    Tested ACK bitcoin@f036dfb on Ubuntu 20.04.
  theStack:
    Code-review ACK f036dfb

Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
20edf4b rpc: Return block time in getblockchaininfo (João Barbosa)

Pull request description:

  Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.

ACKs for top commit:
  naumenkogs:
    ACK 20edf4b
  theStack:
    re-ACK 20edf4b
  0xB10C:
    ACK 20edf4b
  kristapsk:
    ACK 20edf4b
  Zero-1729:
    re-ACK 20edf4b

Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
…y in block chain'

2ebf2fe test: check for RPC error 'Transaction already in block chain' (-27) (Sebastian Falbesoner)

Pull request description:

  This PR adds missing test coverage for the RPC error "Transaction already in block chain" (error code `RPC_VERIFY_ALREADY_IN_CHAIN` = `RPC_TRANSACTION_ALREADY_IN_CHAIN` = -27), which is thrown in the function `BroadcastTransaction` (src/node/transaction.cpp).

ACKs for top commit:
  kristapsk:
    ACK 2ebf2fe (ran linter, looked at changes and ran modified test and checked code in `src/node/transaction.cpp`)
  darosior:
    ACK 2ebf2fe

Tree-SHA512: 8bfbd3ff3da0cb3b8745f69b8ca2377f85fa99f0270750840b60e6ae43b5645c5c59b236993e8b2ad0444ec4171484e4f1ee23fa7e81b79d4222bcb623666fa5
…ransaction

78f4c8b prefer to use txindex if available for GetTransaction (Jameson Lopp)

Pull request description:

  Fixes bitcoin#22382

  Motivation: prevent excessive disk reads if txindex is enabled.

  Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?

ACKs for top commit:
  jonatack:
    ACK 78f4c8b
  theStack:
    Code review ACK 78f4c8b
  LarryRuane:
    tACK 78f4c8b
  luke-jr:
    utACK 78f4c8b
  jnewbery:
    utACK 78f4c8b
  rajarshimaitra:
    Code review ACK bitcoin@78f4c8b
  lsilva01:
    Code Review ACK and Tested ACK bitcoin@78f4c8b on Ubuntu 20.04

Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0
…dationcache_tests

c3e111a Reduced number of validations in `tx_validationcache_tests` to keep the run time reasonable. (lucash-dev)

Pull request description:

  Following a suggestion in the comments, changed `ValidateCheckInputsForAllFlags` from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.

  Timing for `checkinputs_test`:
  ```
  Before:   6.8s
  After:    3.7s
  ----------------
  Saved:    3.1s (45%)
  ```

  This PR was split from bitcoin#13050. Also see bitcoin#10026.

ACKs for top commit:
  leonardojobim:
    tACK bitcoin@c3e111a.
  kallewoof:
    ACK c3e111a
  theStack:
    re-ACK c3e111a

Tree-SHA512: bef49645bdd4f61ec73cc77a9f028b95d9856db9446d2e7fc9a48867a6f0e94c2c9f150cb771a30fe852db0efb0a1bd15d38b00d712651793ccb59ff6157a7b4
…n.cpp

f685a13 doc: GetTransaction()/getrawtransaction follow-ups to bitcoin#22383 (John Newbery)
abc57e1 refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)

Pull request description:

  ~This PR is based on bitcoin#22383, which should be reviewed first~ (merged by now).

  In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.

  Relevant IRC log:
  ```
  17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
  17:53 <raj_> jnewbery, +1
  17:53 <stickies-v> agreed!
  17:54 <glozow> jnewbery ya
  17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
  17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
  17:55 <sipa> (before 0.8, validation itself used the txindex)
  17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
  17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
  17:55 <sipa> jnewbery: sure, just providing background
  17:56 <sipa> seems very reasonable to move it elsewhere now
  ```

  The commit should be trivial to review with `--color-moved`.

ACKs for top commit:
  jnewbery:
    Code review ACK f685a13
  rajarshimaitra:
    tACK bitcoin@f685a13
  mjdietzx:
    crACK f685a13
  LarryRuane:
    Code review, test ACK f685a13

Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
198ceb8 script, doc: guix touchups (jonatack)
d7b7f61 Updated Readme, Corrected the codesign typo (h)

Pull request description:

ACKs for top commit:
  jamesob:
    ACK bitcoin@198ceb8
  jonatack:
    ACK 198ceb8

Tree-SHA512: 408360cebb51cff330fdd5d5d8ae91a168cdc99fb1377913fd9119e6eba536e58f87ff5c5b479e21a21fa3403323b137c338005bbd67e6fd24314929cdff9325
fbeb8c4 test: add type annotations to util.get_rpc_proxy (fanquake)

Pull request description:

  Split out from bitcoin#22092 while we address the functional test failure.

ACKs for top commit:
  instagibbs:
    ACK bitcoin@fbeb8c4

Tree-SHA512: 031ef8703202ae5271787719fc3fea8693574b2eb937ccf852875de95798d7fa3c39a8db7c91993d0c946b45d9b4d6de570bd1102e0344348784723bd84803a8
@PastaPastaPasta PastaPastaPasta merged commit cefa2eb into dashpay:develop Dec 4, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants