Skip to content

Conversation

@codablock
Copy link

No description provided.

@codablock codablock changed the base branch from master to develop September 5, 2018 12:37
@codablock codablock added this to the 12.4 milestone Sep 5, 2018
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.

See in-line, minor formatting

Copy link
Member

Choose a reason for hiding this comment

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

Backet should be on prior line

Allows testing of features which depend on BIP9 deployments
We need to update the CbTx's merkle root for the masternode list here as
tests might add transactions into a block which cause the list to change.
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.

👍

ACK

@UdjinM6 UdjinM6 merged commit 1560b16 into dashpay:develop Sep 5, 2018
@codablock codablock deleted the pr_dip3_unittests branch September 14, 2018 12:51
PastaPastaPasta added a commit that referenced this pull request Aug 20, 2025
, bitcoin#25616, bitcoin#26005, bitcoin#24855, bitcoin#18554, bitcoin#17204, bitcoin#20562, bitcoin#21166, bitcoin#25044, bitcoin#25364, bitcoin#25525, bitcoin#25512, bitcoin#24678, bitcoin#26747, partial bitcoin#22154, bitcoin#24584 (wallet backports: part 8)

c0f9225 merge bitcoin#26747: fix confusing error / GUI crash on cross-chain legacy wallet restore (Kittywhiskers Van Gogh)
d63ca8a merge bitcoin#24678: Prevent wallet unload on GetWalletForJSONRPCRequest (Kittywhiskers Van Gogh)
2a880d8 merge bitcoin#25512: remove wallet dependency and refactor rpc_signrawtransaction.py (Kittywhiskers Van Gogh)
f405790 merge bitcoin#25525: remove wallet dependency from mempool_updatefromblock.py (Kittywhiskers Van Gogh)
c24c9ea merge bitcoin#25364: remove wallet dependency from feature_nulldummy.py (Kittywhiskers Van Gogh)
a118fe1 test: reduce `num_nodes` in `feature_nulldummy.py` to 1 (Kittywhiskers Van Gogh)
c6eabc1 merge bitcoin#25044: Use MiniWallet in rpc_rawtransaction.py (Kittywhiskers Van Gogh)
4e0725e merge bitcoin#21166: Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it (Kittywhiskers Van Gogh)
f883122 merge bitcoin#20562: Test that a fully signed tx given to signrawtx is unchanged (Kittywhiskers Van Gogh)
c349dad merge bitcoin#17204: Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (Kittywhiskers Van Gogh)
c5e8bd6 merge bitcoin#18554: ensure wallet files are not reused across chains (Kittywhiskers Van Gogh)
0fca914 merge bitcoin#24855: Fix `setwalletflag` disabling of flags (Kittywhiskers Van Gogh)
f15a1b9 merge bitcoin#26005: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces) (Kittywhiskers Van Gogh)
240765e merge bitcoin#25616: Return `util::Result` from WalletLoader methods (Kittywhiskers Van Gogh)
bd897fa merge bitcoin#25656: return util::Result from `GetReservedDestination` methods (Kittywhiskers Van Gogh)
56accfe merge bitcoin#25721: Replace BResult with util::Result (Kittywhiskers Van Gogh)
03939f2 partial bitcoin#24584: avoid mixing different `OutputTypes` during coin selection (Kittywhiskers Van Gogh)
50ca8cf refactor: remove `CKey` overload for `Create{,AndProcess}Block()` (Kittywhiskers Van Gogh)
9e23b11 merge bitcoin#25218: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination (Kittywhiskers Van Gogh)
ec764fd partial bitcoin#22154: Add OutputType::BECH32M and related wallet support for fetching bech32m addresses (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * The `CKey` overload for `Create{,AndProcess}Block()` was introduced as a convenience function in [dash#2264](#2264). To allow for the easier backport of [bitcoin#24584](bitcoin#24584) (by resolving an ambiguous overload error) and because `GetScriptForRawPubKey` makes the same construction, it was opted to realign with upstream and drop the overload.

  * Even though we don't actively support any address types that utilize Bech32m, [bitcoin#25656](bitcoin#25656) assumes that `GetReservedDestination` modifies a `bilingual_str` for its error case in order to encapsulate it in a `util::Result`. In order to complete that backport, [bitcoin#22154](bitcoin#22154) has been partially backported.

  * `CKeyHolder` has always assumed that `GetReservedDestination` will succeed without validation of this assumption ([source](https://github.com/dashpay/dash/blob/develop/src/coinjoin/util.cpp#L29-L33)). As [bitcoin#25656](bitcoin#25656) forces us to unwrap the returned value, this assumption has been documented with an `assert` ([source](https://github.com/dashpay/dash/blob/bd897fa7097d3d68d19b8a9af14b23f4007645ca/src/coinjoin/util.cpp#L32-L34))

  * When backporting [bitcoin#21166](bitcoin#21166), we don't have the luxury of a `scriptWitness` to include the redemption script, so `rpc_signrawtransaction.py` must import the script in order to spend it. This creates an additional complication as descriptor wallets do not permit wallets to mix descriptors with and without private keys ([source](https://github.com/dashpay/dash/blob/cc9da5d9e28bf9a7b411dc390523ac6d2dd09de6/src/wallet/rpc/backup.cpp#L1758)) and as P2SH descriptors don't involve a private key, those specific subtests need to be skipped for descriptor wallets.

    * `OP_TRUE` has been appended to the script, this mirrors the `OP_TRUE` in the `scriptWitness` ([source](https://github.com/bitcoin/bitcoin/blob/a97a9298cea085858e1a65a5e9b20d7a9e0f7303/test/functional/rpc_signrawtransaction.py#L270))

  * The subtest `test_signing_with_missing_prevtx_info()` introduced in [bitcoin#25044](bitcoin#25044) requires an extra `walletpassphrase` as the preceding tests that unlock the wallet are not called for descriptor wallets, making the call necessary. It is redundant but harmless for legacy wallets.

  * With the backport of [bitcoin#19937](bitcoin#19937) in [dash#6726](#6726) (as ae7e4cb), we can remove a workaround in `feature_nulldummy.py` where we used two nodes to satisfy `getblocktemplate`'s requirement of a connected node as that requirement was lifted on test chains.

  ## Breaking Changes

  None expected.

  ## How Has This Been Tested?

  A full CoinJoin session run on c0f9225

  ![CoinJoin session run on build c0f9225](https://github.com/user-attachments/assets/1dd31814-8d4a-47ab-95e5-5684818f3971)

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK c0f9225
  UdjinM6:
    utACK c0f9225

Tree-SHA512: 2217281ea75afe3eb6061e1acbd42afb66ff2ab28c15a3ad03bdb528ef4e40ea30bf1adad8d0ba93310a3d561d6c3f2ad95780b0b972af238523c6f2950a39c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants