Skip to content

backport: v0.20 pr5 #4877

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
Merged
PastaPastaPasta merged 13 commits into from
Oct 17, 2022
Merged

backport: v0.20 pr5 #4877

PastaPastaPasta merged 13 commits into from
Oct 17, 2022

Conversation

@Munkybooty
Copy link

@Munkybooty Munkybooty commented Jun 9, 2022

No description provided.

@PastaPastaPasta

This comment was marked as duplicate.

Sign in to view
PastaPastaPasta
PastaPastaPasta requested changes Jun 19, 2022
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.

lint-python fails

@github-actions
Copy link

github-actions bot commented Jun 19, 2022

This pull request has conflicts, please rebase.

@Munkybooty Munkybooty force-pushed the backports-0.20-pr5 branch from b4d6549 to 199a46d Compare June 21, 2022 07:52
@Munkybooty Munkybooty force-pushed the backports-0.20-pr5 branch from 199a46d to 91e92eb Compare June 25, 2022 06:42
@Munkybooty
Copy link
Author

Munkybooty commented Jun 28, 2022 •
edited
Loading

Dropping bitcoin#18407 as it is in #4829

@Munkybooty Munkybooty force-pushed the backports-0.20-pr5 branch from 91e92eb to 64c7ce2 Compare June 28, 2022 16:24
@github-actions
Copy link

github-actions bot commented Jun 30, 2022

This pull request has conflicts, please rebase.

@Munkybooty Munkybooty force-pushed the backports-0.20-pr5 branch from 64c7ce2 to 50aa088 Compare July 6, 2022 21:57
@Munkybooty Munkybooty force-pushed the backports-0.20-pr5 branch 2 times, most recently from 5095c6e to 7ae3e96 Compare August 22, 2022 15:01
@Munkybooty
Copy link
Author

Munkybooty commented Aug 22, 2022

I'm unsure of how to go about fixing mempool_accept.py

@knst
Copy link
Collaborator

knst commented Aug 23, 2022

I'm unsure of how to go about fixing mempool_accept.py

Please, check this fixup commit:
knst@b7f1773

The name of failure in dash's code and in bitcoin code are not exactly same.
"bare-multisig" -> "64: bare-multisig"

@Munkybooty Munkybooty force-pushed the backports-0.20-pr5 branch 2 times, most recently from b9a3719 to 6b86226 Compare August 23, 2022 15:01
UdjinM6
UdjinM6 requested changes Aug 23, 2022
View reviewed changes
@github-actions
Copy link

github-actions bot commented Aug 28, 2022

This pull request has conflicts, please rebase.

UdjinM6
UdjinM6 requested changes Sep 7, 2022
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.

pls see 14f05d8bb22ee90abeb28f4c75d2bebfe137a49f + e9c1ea674ebf149af391cd6c872bebb77800c4e4 to fix 16851 and 561ece03e910d6ab893be3eb47950df64de7d966 + 7b4d2d695cf053d4b07b361f8d424565f999588c to fix 17843

@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 13, 2022
UdjinM6
UdjinM6 previously approved these changes Oct 13, 2022
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

@UdjinM6
Copy link

UdjinM6 commented Oct 16, 2022

@PastaPastaPasta

@Munkybooty Munkybooty requested a review from UdjinM6 October 17, 2022 03:18
@UdjinM6 UdjinM6 modified the milestones: 18.1, 18.2 Oct 17, 2022
UdjinM6
UdjinM6 approved these changes Oct 17, 2022
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.

re-utACK

@PastaPastaPasta PastaPastaPasta added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label Oct 17, 2022
PastaPastaPasta
PastaPastaPasta approved these changes Oct 17, 2022
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 however, a subsequent PR that adds release notes for the added RPC should be completed @Munkybooty

src/wallet/rpcwallet.cpp Outdated
Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 17, 2022

Choose a reason for hiding this comment

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

15930 needs release notes please!

MarcoFalke and others added 13 commits October 17, 2022 15:41
@PastaPastaPasta
Partial Merge bitcoin#17517: ci: Bump to clang-8 for asan build to av…
7d687f9 
…oid segfaults on ppc64le

fa40e48 ci: Remove unparseable lines from supp file for old xenial clang tsan (MarcoFalke)
fa1bfc4 ci: ubsan report_error_type=1 and add suppressions (MarcoFalke)
fa69cef test: Print stderr when subprocess fails (MarcoFalke)
2222c30 test: Use char instead of unsigned char (MarcoFalke)
faa8023 ci: Bump to clang-8 for asan build to avoid segfaults on ppc64le (MarcoFalke)

Pull request description:

  Use clang-8 instead of default clang (which is clang-6 on Bionic) to avoid spurious segfaults when running the ci system on ppc64le

ACKs for top commit:
  practicalswift:
    ACK fa40e48 assuming Travis is happy -- diff looks correct :)

Tree-SHA512: f4f26232d3a0ef38da245869340f723d279a3db9823befbc735fb5a00024dae041c7306d7ae55d2488e6f86aa96cdea155b007aefb561fba505141e8dbc717dc
@PastaPastaPasta
Merge bitcoin#16851: Continue relaying transactions after they expire…
f181b02 
… from mapRelay

168b781 Continue relaying transactions after they expire from mapRelay (Anthony Towns)

Pull request description:

  This change allows peers to request transactions even after they've expired from mapRelay and even if they're not doing mempool requests. This is intended to allow for CPFP of old transactions -- if parent tx P wasn't relayed due to low fees, then a higher fee rate child C is relayed, peers will currently request the parent P, but we prior to this patch, we will not relay it due to it not being in mapRelay.

ACKs for top commit:
  MarcoFalke:
    re-ACK 168b781 (only change is comment fixup)
  sdaftuar:
    re-ACK 168b781
  sipa:
    ACK 168b781

Tree-SHA512: b206666dd1450cd0a161ae55fd1a7eda2c3d226842ba27d91fe463b551fd924b65b92551b14d6786692e15cf9a9a989666550dfc980b48ab0f8d4ca305bc7762
@laanwj @PastaPastaPasta
Merge bitcoin#17910: build: remove double LIBBITCOIN_SERVER linking
6fb0941 
831e122 build: remove double LIBBITCOIN_SERVER linking (fanquake)

Pull request description:

  Seems that this is no longer required. Have tested building on macOS and Debian.

ACKs for top commit:
  promag:
    ACK 831e122.
  practicalswift:
    ACK 831e122
  laanwj:
    ACK 831e122

Tree-SHA512: d226d9fa0292189fae7e2af14781a511c3633f1352324f19ae642e941d06c34e2abf8b1df97d2330d76dba6024a93d8d341e02cc4882d7066f97e82585631fe1
@PastaPastaPasta
Merge bitcoin#17541: test: add functional test for non-standard bare …
038eae1 
…multisig txs

1be0b1f test: add functional test for non-standard bare multisig txs (Sebastian Falbesoner)

Pull request description:

  Approaches another missing functional test of issue bitcoin#17394 (counterpart to unit test in PR bitcoin#17502): A transaction is rejected by the mempool with reason `"bare-multisig"` if any of the outputs' scriptPubKey has bare multisig format (`M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`) and bitcoind is started with the argument `-permitbaremultisig=0`.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@1be0b1f
  kristapsk:
    ACK 1be0b1f

Tree-SHA512: 2cade68c4454029b62278b38d0f137c2605a0e4450c435cdda2833667234edd4406f017ed12fa8df9730618654acbaeb68b16dcabb9f5aa84bad9f1c76c6d476
@laanwj @PastaPastaPasta
Merge bitcoin#17823: scripts: Read suspicious hosts from a file inste…
8c6fb56 
…ad of hardcoding

e1c582c contrib: makeseeds: Read suspicious hosts from a file instead of hardcoding (Sanjay K)

Pull request description:

  referring to: bitcoin#17020
  good first issue: reading SUSPICIOUS_HOSTS from a file.
  I haven't changed the base hosts that were included in the original source, just made it readable from a file.

ACKs for top commit:
  practicalswift:
    ACK e1c582c -- diff looks correct

Tree-SHA512: 18684abc1c02cf52d63f6f6ecd98df01a9574a7c470524c37e152296504e2e3ffbabd6f3208214b62031512aeb809a6d37446af82c9f480ff14ce4c42c98e7c2
@laanwj @PastaPastaPasta
Merge bitcoin#17945: doc: Fix doxygen errors
f94a833 
297e098 Fix doxygen errors (Ben Woosley)

Pull request description:

  These are all the remaining errors identified via -Werror=documentation, e.g.:
  ```
    ./rpc/rawtransaction_util.h:31:13: error: parameter 'prevTxs' not found in the function declaration [-Werror,-Wdocumentation]
      * @param  prevTxs       Array of previous txns outputs that tx depends on but may not yet be in the block chain
                ^~~~~~~
    ./rpc/rawtransaction_util.h:31:13: note: did you mean 'prevTxsUnival'?
      * @param  prevTxs       Array of previous txns outputs that tx depends on but may not yet be in the block chain
                ^~~~~~~
                prevTxsUnival

    netbase.cpp:766:11: error: parameter 'outProxyConnectionFailed[out]' not found in the function declaration [-Werror,-Wdocumentation]
     * @param outProxyConnectionFailed[out] Whether or not the connection to the
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    netbase.cpp:766:11: note: did you mean 'outProxyConnectionFailed'?
     * @param outProxyConnectionFailed[out] Whether or not the connection to the
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              outProxyConnectionFailed
  ```

  You can use this to run with `-Wdocumentation` yourself: bitcoin#14920

ACKs for top commit:
  laanwj:
    ACK 297e098

Tree-SHA512: a232d893b170873d923e77fa56c56a6567e7fd120b5af1f52cfeeae1093eec55621604cc80a523678f6fedc8bbb31228c4aa8dc2a630ce9ffc91525988522af7
@PastaPastaPasta
Merge bitcoin#17480: test: add unit test for non-standard txs with to…
ed70a2f 
…o large scriptSig

5e8a563 test: add unit test for non-standard txs with too large scriptSig (Sebastian Falbesoner)

Pull request description:

  Approaches the first missing test of issue bitcoin#17394: Checks that the function `IsStandardTx()` returns rejection reason `"scriptsig-size"` if any one the inputs' scriptSig is larger than 1650 bytes.

ACKs for top commit:
  MarcoFalke:
    ACK 5e8a563
  instagibbs:
    ACK bitcoin@5e8a563

Tree-SHA512: 79977b12ddea9438a37cefdbb48cc551e4ad02a8ccfaa2d2837ced9f3a185e2e07cc366c243b9e3c7736245e90e315d7b4110efc6b440c63dbef7ee2c9d78a73
@fanquake @PastaPastaPasta
Merge bitcoin#17532: test: add functional test for non-standard txs w…
10d3849 
…ith too large scriptSig

8f2d773 test: add functional test for non-standard txs with too large scriptSig (Sebastian Falbesoner)

Pull request description:

  Approaches another missing functional test of issue bitcoin#17394 (counterpart to unit test in PR bitcoin#17480, Commit bitcoin@5e8a563): A transaction is rejected by the mempool with reason `"scriptsig-size"` if any of the inputs' scriptSig is larger than 1650 bytes.

ACKs for top commit:
  MarcoFalke:
    ACK 8f2d773
  instagibbs:
    ACK bitcoin@8f2d773

Tree-SHA512: 7a45b8a4181158be3e3b91756783ddf032f132ca8780dc35fac91b2df2149268f784d28ac56005135c4d86a357c57805c5a54b8155f0d049932844b18dc03992
@PastaPastaPasta
Merge bitcoin#17502: test: add unit test for non-standard bare multis…
44fec43 
…ig txs

1bb5d51 test: add unit test for non-standard bare multisig txs (Sebastian Falbesoner)

Pull request description:

  Approaches another missing unit test of issue bitcoin#17394: Checks that the function `IsStandardTx()` returns rejection reason `"bare-multisig"` if any one of the outputs' scriptPubKey has bare multisignature format (i.e. `M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`, not P2SH!) and the policy flag `fIsBareMultisigStd` is set to false.

ACKs for top commit:
  instagibbs:
    utACK bitcoin@1bb5d51

Tree-SHA512: d7c95e35da16520d6dcd2b4278e2426fedd13f68d1f23c90e85e929774e123fbfcfbccc26df6ad1c0dd61780896fa4b4b3d4e8280c647bb06df2bfcf2ba572fb
@meshcollider @PastaPastaPasta
Merge bitcoin#17843: wallet: Reset reused transactions cache
3b5b8d0 
6fc554f wallet: Reset reused transactions cache (Fabian Jahr)

Pull request description:

  Fixes bitcoin#17603 (together with bitcoin#17824)

  `getbalances` is using the cache within `GetAvailableCredit` under certain conditions [here](https://github.com/bitcoin/bitcoin/blob/35fff5be60e853455abc24713481544e91adfedb/src/wallet/wallet.cpp#L1826). For a wallet with `avoid_reuse` activated this can lead to inconsistent reporting of `used` transactions/balances between `getbalances` and `listunspent` as pointed out in bitcoin#17603. When an address is reused before the first transaction is spending from this address, the cache is not updated even after the transaction is sent. This means the remaining outputs at the reused address are not showing up as `used` in `getbalances`.

  With this change, any newly incoming transaction belonging to the wallet marks all the other outputs at the same address as dirty.

ACKs for top commit:
  kallewoof:
    Code review re-ACK 6fc554f
  promag:
    ACK 6fc554f.
  achow101:
    Re-ACK 6fc554f
  meshcollider:
    Code review ACK 6fc554f

Tree-SHA512: c4cad2c752176d16d77b4a4202291d20baddf9f27250896a40274d74a6945e0f6b34be04c2f9b1b2e756d3ac669b794969df8f82a98e0b16f10e92f276649ea2
@meshcollider @PastaPastaPasta
Merge bitcoin#17889: wallet: Improve CWallet:MarkDestinationsDirty
ad68327 
2b16414 wallet: Improve CWallet:MarkDestinationsDirty (João Barbosa)

Pull request description:

  Improve `CWallet:MarkDestinationsDirty` by skipping transactions that already have the cache invalidated. Skipping a transaction avoids at worst case extracting all output destinations.

ACKs for top commit:
  meshcollider:
    re-utACK 2b16414

Tree-SHA512: 479dc2dde4b653b856e3d6a0c59a34fe33e963eb131a2d88552a8b30471b8725a087888fe5d7db6e4ee19b74072fe64441497f033be7d1931637f756e0d8fef5
@laanwj @PastaPastaPasta
Merge bitcoin#15930: rpc: Add balances RPC
078094b 
facfb41 rpc: Deprecate getunconfirmedbalance and getwalletinfo balances (MarcoFalke)
999931c rpc: Add getbalances RPC (MarcoFalke)
fad13e9 rpcwallet: Make helper methods const on CWallet (MarcoFalke)
fad40ec wallet: Use IsValidNumArgs in getwalletinfo rpc (MarcoFalke)

Pull request description:

  This exposes the `CWallet::GetBalance()` struct over RPC.

  In the future, incorrectly named rpcs such as `getunconfirmedbalance` or rpcs redundant to this such as `getbalance` could be removed.

ACKs for commit facfb4:
  jnewbery:
    utACK facfb41

Tree-SHA512: 1f54fedce55df9a8ea82d2b6265354b39a956072621876ebaee2355aac0e23c7b64340c3279502415598c095858529e18b50789be956250aafda1cd3a8d948a5
@meshcollider @PastaPastaPasta
Merge bitcoin#16239: wallet/rpc: follow-up clean-up/fixes to avoid_reuse
ccfe4b7 
71d0344 docs: release note wording (Karl-Johan Alm)
3d2ff37 wallet/rpc: use static help text (Karl-Johan Alm)
53c3c1e wallet/rpc/getbalances: add entry for 'mine.used' balance in results (Karl-Johan Alm)

Pull request description:

  This addresses a few remaining issues pointed out in bitcoin#13756:

  * First commit addresses bitcoin#13756 (comment)
  * Second commit addresses bitcoin#13756 (comment)

  Ping jnewbery and achow101 as they pointed out these issues.

ACKs for commit 71d034:
  jnewbery:
    ACK 71d0344
  meshcollider:
    re-utACK bitcoin@71d0344

Tree-SHA512: 5e28822af0574ad07dbbed21aa2fe7866bf5770b4c0a1c150ad0da8af3152bcfb7170330a7497fa500326c594740ecf63733cf58325821e2811d7b911d5783a0
@PastaPastaPasta PastaPastaPasta merged commit ba7d5dc into dashpay:develop Oct 17, 2022
@thephez
Copy link
Collaborator

thephez commented Dec 29, 2022

@Munkybooty Can you update the PR title to include the conventional commit backport: prefix?

@Munkybooty Munkybooty changed the title Backports 0.20 pr5 backport: v0.20 pr5 Dec 30, 2022
@thephez thephez mentioned this pull request Dec 31, 2022
Merged
5 tasks
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

Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Milestone

18.2

Development

Successfully merging this pull request may close these issues.

8 participants

@Munkybooty @PastaPastaPasta @knst @UdjinM6 @thephez @laanwj @fanquake @meshcollider