Commit Graph

26318 Commits

Author SHA1 Message Date
MarcoFalke
5336f42ea8
Merge bitcoin/bitcoin#19801: test: check for all possible OP_CLTV fail reasons in feature_cltv.py (BIP 65)
b01cd9471f435bb36b8ed5211a56baad51111ad2 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner)
dbc19814743cb12960a99793197c811e2750a06b test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner)
8d0ce50c4826529a2d30ffc850bce4d44da6019b test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner)
ce994e1202c4820b1ad5c375d3d671fd0a18e092 test: add tx modfication helper function in feature_cltv.py (Sebastian Falbesoner)

Pull request description:

  The functional test for [BIP65](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) / `OP_CHECKLOCKTIMEVERIFY` (`feature_cltv.py`) currently only tests one out of five conditions that lead to failure of the op-code -- by prepending the script `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to a tx's first input's scriptSig, the case of "_the top item on the stack is less than 0_" is checked:

  f8462a6d27/test/functional/feature_cltv.py (L26-L35)

  This PR adds the other cases (5 in total) by taking an integer argument to the function `cltv_invalidate` that is called in a loop instead of only once per testing scenario. Here is the full list of failure conditions and how they are tested (note that the scriptSig should still be valid before activation of BIP65, when `OP_CLTV` is simply a no-op):
  * _the stack is empty_
  ➡️  prepending `OP_CHECKLOCKTIMEVERIFY` to scriptSig
  * _the top item on the stack is less than 0_
  ➡️  prepending `OP_1NEGATE OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  * _the lock-time type (height vs. timestamp) of the top stack item and the nLockTime field are not the same_
  ➡️  prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  ➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=1296688602 (genesis block timestamp)
  * _the top stack item is greater than the transaction's nLockTime field_
  ➡️  prepending `OPNum(1000) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  ➡️ setting tx.vin[0].nSequence=0 and tx.nCheckTimeLock=500
  * _the nSequence field of the txin is 0xffffffff_
  ➡️  prepending `OPNum(500) OP_CHECKLOCKTIMEVERIFY OP_DROP` to scriptSig
  ➡️ setting tx.vin[0].nSequence=0xffffffff and tx.nCheckTimeLock=500

  The first commit creates a helper function for the tx modification and also includes some tidying up like turning single-line to multi-line Python imports where necessary and cleaning up some PEP8 warnings. The second commit prepares the invalidation function `cltv_invalidate` and the third and the fourth use it and check for the expected reject reason strings ("Operation not valid with the current stack size", "Negative locktime" and "Locktime requirement not satisfied").

ACKs for top commit:
  MarcoFalke:
    review ACK b01cd9471f435bb36b8ed5211a56baad51111ad2 🐣

Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
2024-07-20 00:05:26 +07:00
MarcoFalke
1cc6aa6c83
Merge bitcoin/bitcoin#21691: test: Check that no versionbits are re-used
fa8eaee6a8531db970cc84436bf2ae8150a58642 test: Run versionbits_sanity for all chains (MarcoFalke)
faec1e9ee1f12612831ad5b0f0a767d87bd2d024 test: Address outstanding versionbits_test feedback (MarcoFalke)
fad4167871c3c9fde462e64e3ef3be937e585084 test: Check that no versionbits are re-used (MarcoFalke)

Pull request description:

ACKs for top commit:
  jnewbery:
    Code review ACK fa8eaee6a8531db970cc84436bf2ae8150a58642
  ajtowns:
    ACK fa8eaee6a8531db970cc84436bf2ae8150a58642 code review only

Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
2024-07-20 00:05:26 +07:00
MarcoFalke
b55fdf8e68
Merge #21235: p2p: Clarify disconnect log message in ProcessGetBlockData, remove send bool
fa8177324392c923c6ce39056cfd870af55ab673 style-only: Remove whitespace (MarcoFalke)
fae77b9e6dc9e59b355d56df49c4d9685b6f40a4 net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman)
fae7c0429f96e08bcac944f6fa30264636dfda8c log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects (MarcoFalke)

Pull request description:

  * Clarify that "ignoring" really means "disconnect" in the log
  * Revive a refactor I took from #13670

ACKs for top commit:
  jnewbery:
    utACK fa8177324392c923c6ce39056cfd870af55ab673
  sipa:
    utACK fa8177324392c923c6ce39056cfd870af55ab673

Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
2024-07-20 00:05:26 +07:00
fanquake
ddc6fca7f3
Merge #21343: doc: revamp macOS build doc
c180c911b88b2bd2baf2c9c2b24e276787ffb69b doc: revamp macOS build doc (Jarol Rodriguez)

Pull request description:

  This PR makes the macOS build-docs more informative and adds in the following information:
  - Proper descriptions and delineation of required/optional dependencies
  - walk-through of optional dependencies
  - configuration walk-through
  - various other tidbits of information

  This is a part of the efforts done in https://github.com/bitcoin/bitcoin/pull/20601 and https://github.com/bitcoin/bitcoin/pull/20610 to update the docs and introduce some consistency between them.

  This update does not add instructions for arm-based M1 Macbooks as I do not have one to test with. It would be nice to have someone follow up with an update containing instructions for arm-based Macs.

  **Before/Master:** [render](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md)
  **After/PR:** [render](c180c911b8/doc/build-osx.md)

ACKs for top commit:
  fanquake:
    ACK c180c911b88b2bd2baf2c9c2b24e276787ffb69b - I still think these are getting too verbose and we're duplicating information all over the place; dependencies, configure options, combinations of options etc. However if people are happy to maintain them, I guess it's fine for now, and this revamping has already happened for some of the other build READMEs.

Tree-SHA512: 1440046c723fe80d4158e4a429e3aa8bd93570acb84ad202d5d24c749ab9a89a3aca8b61b49e75e042a4bf4317acd632d3906e1b5808a9052e74209256528b45
2024-07-20 00:05:26 +07:00
Wladimir J. van der Laan
709652bff7
Merge #21141: wallet: Add new format string placeholders for walletnotify
06e1fb0b170a69996a7ce1ef5203785a7bc6b278 Add new format string placeholders for walletnotify to include relevant block information for transactions (Maayan Keshet)

Pull request description:

  This patch includes two new format placeholders for walletnotify:
  %b - the hash of the block containting the transaction (zeroed if a mempool transaction)
  %h - the height of the block containing the transaction (zero if a mempool transaction)

  I've included test suite changes to check and validate the above functional requirements as well as doc/help description changes.

  **Motivation**
  The walletnotify option is used to be notified of new transactions relevant to the wallet of the node.
  A common usage pattern is to perform afterwards additional RPC calls to determine:
  1. If this is a mempool transaction or not (i.e. are there any confirmations?)
  2. What block was it included in?
  3. Did this transaction was seen before and is now seen again because of a fork?

  All of these questions can be answered with the current features, but the resulting RPC calls may be expensive in a heavily used node. As this information is readily available when calling the walletnotify callback, it makes sense to save expensive round trips by optionally sending this information at that point in time. I can definitely say we would like to use it in Fireblocks, my employer.

  Please let me know of any questions and suggestions.

ACKs for top commit:
  laanwj:
    ACK 06e1fb0b170a69996a7ce1ef5203785a7bc6b278

Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
2024-07-20 00:05:26 +07:00
MarcoFalke
a3bee9c8ec
Merge #21424: Net processing: Tidy up CNodeState ctor
6927933782acb9b158787e6f35debb916793f6b1 [net processing] Add ChainSyncTimeoutState default initializers (John Newbery)
55966e0cc03f0e380d21a9434b048d4d515b6729 [net processing] Remove CNodeState ctor body (John Newbery)

Pull request description:

  This addresses the two outstanding review comments from #21370.

ACKs for top commit:
  practicalswift:
    cr ACK 6927933782acb9b158787e6f35debb916793f6b1: patch looks correct
  hebasto:
    ACK 6927933782acb9b158787e6f35debb916793f6b1, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: b3ef5c8a096e447887df255406b3a760f01c73e2b942374595416b4b4031fc69b89cd93168c45040489d581f340b2a62d3fbabd207d4307f587c00a7a7daacd1
2024-07-20 00:05:26 +07:00
fanquake
aab2a665c3
Merge #20556: rpc: Properly document return values (submitblock, gettxout, getblocktemplate, scantxoutset)
fa7ff0790ec21d187f1a32310918b0c8d66e5266 rpc: Properly document submitblock return value (MarcoFalke)
fae542c28b269d4a8a39f48ef829734b1241dd4f rpc: Properly document getblocktemplate return value (MarcoFalke)
fabaccf031cfac718bf05b140f1901d93ee8be67 rpc: Properly document scantxoutset return value (MarcoFalke)
faa2059547389e342991ab0d9382f8694f74fce1 rpc: Properly document gettxout return value (MarcoFalke)

Pull request description:

  Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476

ACKs for top commit:
  fjahr:
    utACK fa7ff0790ec21d187f1a32310918b0c8d66e5266
  amitiuttarwar:
    tACK fa7ff0790e

Tree-SHA512: 933cb8f003163d93dbedb302d4c162514c2698ec6d58dbb9a053da8b8b9a4459b0701a3d9e830ecdabd7f278a46b7a07a3af49ec60703a80fcd75390877294ea
2024-07-20 00:05:26 +07:00
MarcoFalke
c26722fc0e
Merge #21331: rpc: replace wallet raw pointers with references (#18592 rebased)
7c90c67b7e6f598f9ffdc136ded2b533b78ed044 rpc: refactor rpc wallet functions to take references instead of pointers (fanquake)
48669340080feaff86b8fc0403ef22c820477697 rpc: remove calls to CWallet.get() (fanquake)

Pull request description:

  This is a rebased #18592.

  > This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here https://github.com/bitcoin/bitcoin/issues/18590

  > It seems that this PR is indirectly related to this issue: https://github.com/bitcoin/bitcoin/pull/13063#discussion_r186740049

  > Notice: I have deliberately **not** changed the class `WalletRescanReserver ` whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral".

  > Fixes https://github.com/bitcoin/bitcoin/issues/18590

ACKs for top commit:
  MarcoFalke:
    ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044 🐧
  ryanofsky:
    Code review ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044. Changes easy to review with `--word-diff-regex=. -U0`

Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
2024-07-20 00:05:20 +07:00
pasta
debc7068cf
Merge #6109: fix: fix depends builds on FreeBSD hosts
bbc99571f3 fix: sidestep c++17 std::unary_function removal by compiling boost with c++11 (Kittywhiskers Van Gogh)
3c622a3916 revert: partial dash#5610 (make depends compilable with Xcode 15 on macos) (Kittywhiskers Van Gogh)
f15e1db477 fix: make `std::unary_function` suppression flag no longer contingent on `--enable-suppress-external-warnings` (Kittywhiskers Van Gogh)
5db84e2a5b revert: partial dash#3003 (Fix 2 common Travis failures which happen when Travis has network issues) (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  Despite builders for BSD-based platforms being backported as early as [dash#5362](https://github.com/dashpay/dash/pull/5362), they didn't work on account of using a `curl` flag, `--retry`, that was lopped off other builders in [dash#3003](https://github.com/dashpay/dash/pull/3003) as a way to mitigate aberrant Travis CI-specific behaviour.

  As the variable `$(DOWNLOAD_RETRIES)` was removed as part of that mitigation, the introduction of builders that rely on this variable caused failures as the flag was accompanied by nothing. As our CI host isn't based on FreeBSD, this went unnoticed. When deciding between extending the existing patch and reverting it, reverting it proved to be more attractive on account of us no longer using Travis CI and the revert bringing us closer to upstream.

  Additionally, the `std::unary_function` patch that was introduced in [dash#5610](https://github.com/dashpay/dash/pull/5610) proves to be necessary on BSD-based platforms as well (had to be extended for and tested on GhostBSD, based on FreeBSD 13.2-STABLE, Clang 16). Instead of conditionally patching based on platform and compiler, a more reliable patch would be to downgrade the C++ version used to build Boost at the last version to have `std::unary_function`, which would be C++11, albeit deprecated ([source](https://en.cppreference.com/w/cpp/utility/functional/unary_function)).

  Though it's likely this route wasn't taken originally due to compiler errors that happened despite downgrading to C++11. These errors were due to the compiler objecting to `std::unary_function` usage in Boost headers, despite the backport of [bitcoin#25436](https://github.com/bitcoin/bitcoin/pull/25436), which should've solved this problem. The reason the errors were still persisting is because the necessary flag, `-DBOOST_NO_CXX98_FUNCTION_BASE`, was only applied if `--enable-suppress-external-warnings` was set. CI didn't catch this, as the flag is always set, to keep log lengths manageable. This has been rectified.

  All changes combined, one should be able to build non-Qt Dash binaries using `depends` though building the Qt client from `depends`  unfortunately remains a problem, even upstream ([source](https://github.com/bitcoin/bitcoin/pull/23955#issuecomment-1039300427), [source](https://github.com/bitcoin/bitcoin/pull/23948#issue-1092284497)).

  ## Demo

  **Based on bbc99571f36ecfd817dc33b883c5e6f120240270**

  Built using:
  * **`depends` flags:** `NO_QT=1 ALLOW_HOST_PACKAGES=1 HOST=x86_64-unknown-freebsd13.2`
  * **`configure` flags :** `--prefix=$(pwd)/depends/x86_64-unknown-freebsd13.2 --enable-debug --enable-suppress-external-warnings --with-gui`
  * Qt installed using `pkg` (`qt5`)

  ![Dash-Qt running on GhostBSD](https://github.com/dashpay/dash/assets/63189531/608ff7e6-0e53-41a6-92dd-e31ab2c76e2e)

  ## Checklist:

  - [x] I have performed a self-review of my own code **(note: N/A)**
  - [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 **(note: N/A)**
  - [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:
  UdjinM6:
    light ACK bbc99571f3
  PastaPastaPasta:
    utACK bbc99571f3
  knst:
    ACK bbc99571f3

Tree-SHA512: b29d6775f42965d2f09307aff0192012aa192e39e06a83b800613831dc00fc7173201d496117fbe86850f5208a0b7688a376f2ee618881c062c28d694085efc9
2024-07-19 11:39:29 -05:00
pasta
bf0df81a73
Merge #6117: docs: update supported versions in SECURITY.md
878bce0f45 docs: update SECURITY.md supported versions (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  Updates the supported versions list in `SECURITY.md`

  ## Checklist:

  - [x] I have performed a self-review of my own code **(note: N/A)**
  - [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 **(note: N/A)**
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 878bce0f45
  UdjinM6:
    utACK 878bce0f45
  knst:
    utACK 878bce0f45

Tree-SHA512: d941a3ca0792b2f08f68cab562a35d869d8e93f627918a25a9753955b6103d1515899b0ca50ff936c966b9f9fd603e12d27b03267361c8f1030a31f9fffdf2ae
2024-07-19 11:38:10 -05:00
pasta
e07431b7d3
Merge #6110: backport: bitcoin#18531, #20012, #21035, #21572, #21574 (rpc command) and related fixes
bfc083e9b7 Merge #21574: Drop JSONRPCRequest constructors after #21366 (MarcoFalke)
c0cdb0488b Merge #21572: Fix wrong wallet RPC context set after #21366 (MarcoFalke)
2f7814acdd Merge #21035: Remove pointer cast in CRPCTable::dumpArgMap (MarcoFalke)
d3b1ef374c refactor: simplify implementation of RPC composite commands (Konstantin Akimov)
3270becc9b chore: add TODO to make client parsing for composite commands (Konstantin Akimov)
d55759fa79 Merge #20012: rpc: Remove duplicate name and argNames from CRPCCommand (Wladimir J. van der Laan)
1d87ce4e86 Merge #18531: rpc: remove deprecated CRPCCommand constructor (MarcoFalke)
a7e538d7ae fix: missing changes from bitcoin#19250 (Konstantin Akimov)
68c5da41dc feat: fix help message - all subcommands support it now! (Konstantin Akimov)
d3e181f516 fix: add missing client's argument parsing for RPC commands (Konstantin Akimov)
37bd4009c1 refactor: use monostate instead std::optional in CoreContext (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Backports from bitcoin v22 rpc command related

  ## What was done?
  See commits for backports.
  Also:
   - refactored and significantly simplified implementation of composite commands
   - added missing changes from bitcoin#19250
   - fix  help message for rpc `help` - all subcommands support it now
   - add missing client's argument parsing for RPC commands
   - CoreContext uses std::monostate instead nullopt which is best-practice for std::variant

  ## How Has This Been Tested?
  Run unit/functional tests.
  Checked autocomplete for various commands
  Checked help for various commands

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

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

Tree-SHA512: b7b586eac2f848a6808c677252a5965577bc783778fd70d3025b8d510113de2b177d423d72ea5f61ddd8905673bf3458e55810ada371ee235fbaa19de8d2d36f
2024-07-19 11:35:58 -05:00
pasta
f5bf5ce77a
Merge #6116: fix: mitigate crashes associated with some upgradetohd edge cases
69c37f4ec2 rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD` (Kittywhiskers Van Gogh)
619b640a77 wallet: unify HD chain generation in CWallet (Kittywhiskers Van Gogh)
163d31861c wallet: unify HD chain generation in LegacyScriptPubKeyMan (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  When filming demo footage for https://github.com/dashpay/dash/pull/6093, I realized that if I tried to create an encrypted blank legacy wallet and run `upgradetohd [mnemonic]`, the client would crash.

  ```
  dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt
  QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash'
  dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed.
  Posix Signal: Aborted
  No debug information available for stacktrace. You should add debug information and then run:
  dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa===
  ```

  The expected set of operations when performing privileged operations is to first use `walletpassphrase [passphrase] [time]` to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., `upgradetohd` imports **always** need the passphrase, if encrypted).

  You might have noticed that I used `upgradetohd [mnemonic]` instead of the correct syntax, `upgradetohd [mnemonic] "" [passphrase]` that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told.

  ```
  Error: Please enter the wallet passphrase with walletpassphrase first.
  ```

  Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so `EnsureWalletIsUnlocked()` is happy, so now the following happens.

  ```
  upgradetohd()
    | Insufficient validation has allowed us to supply a blank passphrase
    | for an encrypted wallet
    |- CWallet::UpgradeToHD()
      |- CWallet::GenerateNewHDChainEncrypted()
       | We get our hands on vMasterKey by generating the key from our passphrase
       | and using it to unlock vCryptedMasterKey.
       |
       | There's one small problem, we don't know if the output of CCrypter::Decrypt
       | isn't just gibberish. Since we don't have a passphrase, whatever came from
       | CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the
       | vMasterKey we just got is gibberish
       |- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain()
         |- res = LegacyScriptPubKeyMan::EncryptHDChain()
         | |- EncryptSecret()
         |   |- CCrypter::SetKey()
         |      This is where everything unravels, the gibberish key's size doesn't
         |      match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out.
         |- assert(res)
            We assume are inputs are safe so there's no real reason we should crash.
            Except our inputs aren't safe, so we crash. Welp! :c
  ```

  This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (19512988c6), trying to do the same thing would return you a vague error

  ```
  Failed to generate encrypted HD wallet (code -4)
  ```

  In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to `upgradetohd`, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash.

  ```
  upgradetohd()
   | We've been supplied a passphrase, so we can try and validate it by
   | trying to unlock the wallet with it. If it fails, we know we got the
   | wrong passphrase.
   |- CWallet::Unlock()
   | | Before we bother unlocking the wallet, we should check if we're
   | | already unlocked, if we are, we can just say "unlock successful".
   | |- CWallet::IsLocked()
   | |  Wallet is indeed unlocked.
   | |- return true;
   | The validation method we just tried to use has a bail-out mechanism
   | that we don't account for, the "unlock" succeded so I guess we have the
   | right passphrase.
   [...] (continue call chain as mentioned earlier)
         |- assert(res)
            Oh...
  ```

  This pull request aims to resolve crashes caused by the above two edge cases.

  ## Additional Information

  As this PR was required me to add additional guardrails on `GenerateNewCryptedHDChain()` and `GenerateNewHDChainEncrypted()`, it was taken as an opportunity to resolve a TODO ([source](9456d0761d/src/wallet/wallet.cpp (L5028-L5038))). The following mitigations have been implemented.

  * Validating `vMasterKey` size (any key not of `WALLET_CRYPTO_KEY_SIZE` size cannot be used for encryption and so, cannot be a valid key)
  * Validating `secureWalletPassphrase`'s presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase)
  * Using `Unlock()` to validate the correctness of `vMasterKey`. (the two other instances of iterating through `mapMasterKeys` use `Unlock()`, see [here](1394c41c8d/src/wallet/wallet.cpp (L5498-L5500)) and [here](1394c41c8d/src/wallet/wallet.cpp (L429-L431)))
    * `Lock()`'ing the wallet before `Unlock()`'ing the wallet to avoid the `IsLocked()` bail-out condition and then restoring to the previous lock state afterwards.
  * Add an `IsCrypted()` check to see if `upgradetohd`'s `walletpassphrase` is allowed to be empty.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [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:
  knst:
    utACK 69c37f4ec2
  UdjinM6:
    utACK 69c37f4ec2
  PastaPastaPasta:
    utACK 69c37f4ec2

Tree-SHA512: 4bda1f7155511447d6672bbaa22b909f5e2fc7efd1fd8ae1c61e0cdbbf3f6c28f6e8c1a8fe2a270fdedff7279322c93bf0f8e01890aff556fb17288ef6907b3e
2024-07-19 11:33:58 -05:00
pasta
5f9f05edbc
Merge #6119: fix: move g_txindex initialization out of erroneous location and into constructor
77915de40a test: run `Interrupt()` before `Stop()`, add additional sanity check (Kittywhiskers Van Gogh)
a376e9357a fix: init `g_txindex` only once, downgrade from assert to exception (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  `g_txindex` should be initialized in `TestChainSetup`'s constructor but when backporting [bitcoin#19806](6bf39d7632 (diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R289)) ([dash#5236](https://github.com/dashpay/dash/pull/5236)), portions of the constructor were split into `TestChainSetup::mineBlocks()`, `g_txindex`'s init was left behind in the latter instead of the former.

  This meant that every `mineBlocks()` call would re-create a `TxIndex` instance, which is not intended behaviour; and was recorded to cause `heap-use-after-free`s ([comment](https://github.com/dashpay/dash/pull/6085#issuecomment-2228109300), also the reason this PR was opened).

  This PR aims to resolve that.

  ## Additional Information

  * Crashes stemming from previous attempts (except for one attempt) were not reproducible with my regular local setup (`depends` built with Clang 16, Dash Core built with Clang 16, set of debug-oriented flags, unit tests run using `./src/test/test_dash`).
    * Attempting to rebuild Dash Core with GCC 9 was insufficient, required to rebuild depends with GCC 9 as well
    * `configure`'d with `CC=gcc CXX=g++ CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20 --disable-ccache`
    * Unit tests must be run with `make check-recursive -j$(( $(nproc --all) - 2 ))`
  * An index must be initialized **after** the chain is constructed, this seems to be corroborated by all other index usage ([source](09239a17c7/src/test/blockfilter_index_tests.cpp (L141)), [source](09239a17c7/src/test/coinstatsindex_tests.cpp (L33)), [source](09239a17c7/src/test/txindex_tests.cpp (L31)), all three use `Start()` for their respective indexes _after_ `TestChain100Setup`'s constructor runs `mineBlocks()`)
    * Attempting to run `Start()` earlier (_before_ the `mineBlocks()` call in the constructor) results in erratic behaviour
    * This also explains why my attempt at moving it back to `TestingSetup` (a grandparent of `TestChainSetup`) failed
  * `Interrupt()` is supposed to be called before `Stop()` but this was erroneously removed in a [commit](cc9dcdd0e0 (diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L413-L419)) that adopted `IndexWaitSynced`. This has since been resolved.
  * In line [with](09239a17c7/src/test/blockfilter_index_tests.cpp (L138-L139)) [other](09239a17c7/src/test/coinstatsindex_tests.cpp (L29-L31)) [indexes](09239a17c7/src/test/txindex_tests.cpp (L28-L29)), an sanity check has been added. Additionally, as `TxIndex::Start()` is more akin to `CChainState::LoadGenesisBlock()` than `CChainState::CanFlushToDisk()`, the `assert` has been downgraded to an exception.

  ## 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 **(note: N/A)**
  - [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:
  knst:
    utACK 77915de40a
  UdjinM6:
    utACK 77915de40a
  PastaPastaPasta:
    utACK 77915de40a

Tree-SHA512: 5051dcf62b6cad4597044b4d27dc54c5bafa8692ba31e03c8afc0f7ef80b790a3873cf0275bb2cf6260939a11f95625da79fc7987951e66114900a86276c037c
2024-07-19 11:16:37 -05:00
pasta
5407f5e499
Merge #6120: docs: fix missing 'apt get install', make build steps more prominent, format consistently
185e0fa1e8 doc: consistently use ```sh tags for correct shell highlighting (AJ ONeal)
3c198e01b8 doc: consistently use 'apt-get install' rather than 'apt install' (due to 'apt's sutble dependencies on interactive environment) (AJ ONeal)
22f7596b12 doc: add missing 'install' to 'apt-get' command (AJ ONeal)
ea97482430 doc: make build steps more prominent (AJ ONeal)

Pull request description:

  Developers come to Dash should have clear instruction for how to build / compile just by scanning the README.

  Also, the instructions should be correct and consistent.

  ## What was done?

  - `apt-get` => `apt-get install` (fix typo)
  - `apt install` => `apt-get install` (due to subtle bugs with `apt` in non-interactive scripts)
  - correct grammar `doc folder` => `./doc/`
  - make build / compile documentation prominent in README.md \
    (easy to find via ctrl+f or scanning)
  - use `` ```sh `` consistently for shell formatting (rather than mixing ``     ``, `` ```bash ``, `` ```shell ``, and `` ```sh ``

  ## How Has This Been Tested?

  See <https://github.com/dashhive/dash/tree/doc-build-docs>.

  Note that it's formatted well and consistently and the typos are fixed.

  ## Breaking Changes

  N/A

  Only fixes here.

  ## Checklist:

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

ACKs for top commit:
  UdjinM6:
    utACK 185e0fa1e8
  PastaPastaPasta:
    utACK 185e0fa1e8

Tree-SHA512: 5d05928568cdbc91dbce8090095f55e39c2453cc24dd2350333159e156efda31eac2d8a5ed5b39dc23eb9973057df60b1423e06077ee1bc2269d6235ed70c67a
2024-07-18 11:01:03 -05:00
pasta
0702f89959
Merge #6125: docs: update manpages for 21.0
e2f56de7f4 docs: update manpages for 21.0 (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  https://github.com/dashpay/dash/issues/6081

  ## What was done?
  run `./contrib/devtools/gen-manpages.sh`, sanitize version name

  ## How Has This Been Tested?
  n/a

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

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

Tree-SHA512: 9b56f7a31279457aed1b7ed0b627d4364f786948f6df3176e24ab73b68d785fc90d9bf6136d7965c1c5b97b589b4d228edd338e666d1999e841c6e544f054c05
2024-07-18 10:57:35 -05:00
pasta
ea6f41198d
Merge #6126: feat: enable EHF activation of MN_RR on mainnet
a8a3ea0e90 feat: enable EHF activation of MN_RR on mainnet (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  https://github.com/dashpay/dash/issues/6081

  ## What was done?
  Removed a code, that disabled MN_RR activation with EHF on Main Net

  ## How Has This Been Tested?
  This code is tested on devnet, is in process of testing on testnet.

  ## Breaking Changes
  It make MN_RR possible to get active on mainnet.

  ## Checklist:
  - [x] 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
  - [x] I have assigned this pull request to a milestone

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

Tree-SHA512: 0ae7aecca8463436e952154210cf564333cd77dd1149f7ff88ca144f3b7c434e75e473ea3a6850da1b126efd8a9cece34e46b4bf2b37f5937bcf1f5780e18f50
2024-07-18 10:55:03 -05:00
UdjinM6
85762dc633
60%+: bg, ro, vi 2024-07-18 13:35:16 +03:00
UdjinM6
2d0e68dcd6
80%+: ar, de, es, fi, fr, it, ja, ko, nl, pl, pt, ru, sk, th, tr, zh_CN, zh_TW 2024-07-18 13:35:16 +03:00
UdjinM6
f7992b0b03
en 2024-07-18 12:48:03 +03:00
UdjinM6
49fc976121
dashstrings 2024-07-18 12:45:48 +03:00
UdjinM6
c8333a59c5
chore: replace remaining ... with in translated strings 2024-07-18 12:45:15 +03:00
Hennadii Stepanov
ac2e9ea1e7
qt: Extract translations correctly from UTF-8 formatted source 2024-07-18 12:38:44 +03:00
AJ ONeal
185e0fa1e8
doc: consistently use ```sh tags for correct shell highlighting 2024-07-18 01:26:14 -06:00
AJ ONeal
3c198e01b8
doc: consistently use 'apt-get install' rather than 'apt install' (due to 'apt's sutble dependencies on interactive environment) 2024-07-18 01:24:32 -06:00
AJ ONeal
22f7596b12
doc: add missing 'install' to 'apt-get' command 2024-07-18 01:24:30 -06:00
AJ ONeal
ea97482430
doc: make build steps more prominent 2024-07-18 01:22:42 -06:00
Konstantin Akimov
a8a3ea0e90
feat: enable EHF activation of MN_RR on mainnet 2024-07-18 12:48:55 +07:00
Konstantin Akimov
e2f56de7f4
docs: update manpages for 21.0
run `./contrib/devtools/gen-manpages.sh`, sanitize version name
2024-07-18 12:46:31 +07:00
Kittywhiskers Van Gogh
77915de40a
test: run Interrupt() before Stop(), add additional sanity check 2024-07-18 02:13:28 +00:00
Kittywhiskers Van Gogh
a376e9357a
fix: init g_txindex only once, downgrade from assert to exception
`g_txindex` should be initialized in `TestChainSetup`'s constructor but
in bitcoin#19806 (dash#5236), when portions of the constructor were
split into `mineBlocks()`, `g_txindex`'s init was left behind in the
latter instead of the former. This meant that every `mineBlocks()` call
would re-create a `TxIndex` instance, which is not intended behaviour.

Also, a runtime exception is more appropriate and closer to the usage of
`BOOST_REQUIRE` in other index `Start()` calls than the harsher `assert`
2024-07-18 02:13:28 +00:00
pasta
47e3197940
Merge #6118: docs: add release notes notifying change of default branch to develop
8a66af25e8 docs: add release notes notifying change of default branch to `develop` (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  Add a release note notifying the new default branch as `develop`.

  ## Checklist:

  - [x] I have performed a self-review of my own code **(note: N/A)**
  - [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 **(note: N/A)**
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 8a66af25e8
  UdjinM6:
    utACK 8a66af25e8

Tree-SHA512: 82212ce670cf4b0f243a79170914ad04b1d118406ce6402b33dfb42a5ae0865c36de4b816530238bb9ded796c66f3dcc36fa9400ace59b6e7dad24ba47653e4f
2024-07-17 21:09:41 -05:00
Kittywhiskers Van Gogh
69c37f4ec2
rpc: make sure upgradetohd always has the passphrase for UpgradeToHD
earlier it was possible to make it all the way to `EncryptSecret`
without actually having the passphrase in hand until being told off
by `CCrypter::SetKey`, we should avoid that.

also, let's get rid of checks that `UpgradeToHD` is now taking
responsibility for. no point in checking if the wallet is unlocked
as it has no bearing on your ability to upgrade the wallet.
2024-07-17 16:31:33 +00:00
Kittywhiskers Van Gogh
619b640a77
wallet: unify HD chain generation in CWallet
additionally, let's do the lock-unlock validation test *before* trying
to generate a new chain. as a bonus, it lets us make sure that we
encrypt the HD chain using a key that's actually capable of unlocking
the wallet.

also, `upgradetohd` never checked if the passphrase was handed to us
(which matters in encrypted blank wallets) before calling UpgradeToHD,
so let's check it for them.
2024-07-17 16:31:32 +00:00
Kittywhiskers Van Gogh
163d31861c
wallet: unify HD chain generation in LegacyScriptPubKeyMan
most of the steps are overlapping except for the encryption sections,
which we can make contingent on supplying of the keying material. `res`
isn't used anywhere since we plan on hard crashing if they fail anyway,
so if we got that far, we've already succeeded.

we will validate vMasterKey's size before starting the encryption
routine to prevent a value outside of our control from hard-crashing
the client.
2024-07-17 16:31:30 +00:00
UdjinM6
121c032e41
fix: avoid calling functions that change wallet state inside of assert(...) 2024-07-17 18:02:08 +03:00
pasta
09239a17c7
Merge #6108: fix: remove keypool replenishment stat and warning for descriptor wallet
b7c7bff6e0 rpc: remove keypool replenishment stat and warning for descriptor wallet (Kittywhiskers Van Gogh)

Pull request description:

  ## Breaking Changes

  - `getcoinjoininfo` will no longer report `keys_left` and will not incorrectly warn about keypool depletion with descriptor wallets

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [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 **(note: N/A)**
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK b7c7bff6e0
  knst:
    utACK b7c7bff6e0
  UdjinM6:
    re-utACK b7c7bff6e0
  PastaPastaPasta:
    utACK b7c7bff6e0

Tree-SHA512: d6e55698a30288308e206566bafb7ea4cd465f37c3f6975410ff15b246500e576ee006da712d28eaa1d4845451f19b218e8c4d11f311b860ed674ad52dca1e0c
2024-07-16 12:37:44 -05:00
Kittywhiskers Van Gogh
8a66af25e8
docs: add release notes notifying change of default branch to develop 2024-07-16 16:48:31 +00:00
Kittywhiskers Van Gogh
878bce0f45
docs: update SECURITY.md supported versions 2024-07-16 15:32:39 +00:00
pasta
f16025f735
Merge #6094: feat: support descriptor wallets for RPC governance votemany, votealias
c72ec70fdf feat: implement governance RPCs votealias and votemany for descriptor wallets (Konstantin Akimov)
490832959d refactor: new method to generate a signing message in CGovernanceVote (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  RPCs `governance votemany` and `governance votealias` use forcely LegacyScriptPubKeyMan instead using CWallet's interface.
  It causes a failures such as
  ```
  test_framework.authproxy.JSONRPCException: This type of wallet does not support this command (-4)
  ```
  See https://github.com/dashpay/dash-issues/issues/59 to track progress

  ## What was done?
  Use CWallet's interfaces instead LegacyScriptPubKeyMan

  ## How Has This Been Tested?
  Functional tests `feature_governance.py` and `feature_governance_cl.py` to run by both ways - legacy and descriptor wallets.

  Run unit and functional tests.

  Extra test done locally:
  ```diff
  --- a/test/functional/test_framework/test_framework.py
  +++ b/test/functional/test_framework/test_framework.py
  @@ -242,10 +242,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):

           if self.options.descriptors is None:
               # Prefer BDB unless it isn't available
  -            if self.is_bdb_compiled():
  -                self.options.descriptors = False
  -            elif self.is_sqlite_compiled():
  +            if self.is_sqlite_compiled():
                   self.options.descriptors = True
  +            elif self.is_bdb_compiled():
  +                self.options.descriptors = False
  ```

  to flip flag descriptor wallets/legacy wallets for all functional tests.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

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

Tree-SHA512: 2c18f0d4acb1c4d57da81bf54f0d155682f558eeb7271df7e6fe75c126ef7f047562794a6730e3ca5351abc4e2daded06b874c2ab77f9c47b840c89d8a158c9f
2024-07-16 09:32:34 -05:00
pasta
b0b6ba11c1
Merge #6115: backport: v21.0.x rc.2
db828177bf Merge #6106: feat: create new composite quorum-command platformsign (pasta)
a45e6df58b Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta)
7330982631 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta)
9998ffd92b Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta)
cdf7a25012 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta)
c1c2c55690 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta)
117548660d Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Backports a set of 6 PRs needed in rc.2

  ## What was done?
  Backported PRs with labels

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  kwvg:
    LGTM, utACK db828177bf
  UdjinM6:
    utACK db828177bf

Tree-SHA512: 1b242c5db04bd5873ef622543bc2a25e29567f15962c677ea51ff05cb784291968d18f419bf611c206b912e8f15d687208ae75af33aab89038b6f0167d99c4bf
2024-07-15 17:09:14 -05:00
MarcoFalke
bfc083e9b7
Merge #21574: Drop JSONRPCRequest constructors after #21366
9044522ef76f880760165d98fab024802ccfc062 Drop JSONRPCRequest constructors after #21366 (Russell Yanofsky)

Pull request description:

  This just makes an additional simplification after #21366 replaced
  util::Ref with std::any. It was originally suggested
  https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351 but
  delayed for a followup. It would have prevented usage bug
  https://github.com/bitcoin/bitcoin/pull/21572.

ACKs for top commit:
  promag:
    ACK 9044522ef76f880760165d98fab024802ccfc062, fixed conflict in src/wallet/interfaces.cpp.

Tree-SHA512: e909411b8f75013620b94e1a609296befb832fdcb574cd2e6689bfe3c636b03cd4ac1ccb2b32b532daf0f2131bb043464024966310fffc7e3cad77713d4bd0ef
2024-07-16 00:25:06 +07:00
MarcoFalke
c0cdb0488b
Merge #21572: Fix wrong wallet RPC context set after #21366
937fd4a66f048780bffc5e714d0c800de987ce93 Fix wrong wallet RPC context set after #21366 (Russell Yanofsky)

Pull request description:

  This bug doesn't have any effects currently because it only affects
  external signer RPCs which aren't currently using the wallet context,
  but it does cause an appveyor failure in a upcoming PR:

  https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882

  This bug is subtle and could have been avoided if JSONRPCRequest didn't
  have constructors that were so loose with type checking.  Suggested
  change
  https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351
  eliminates these and would be a good followup for a future PR.

  This PR just implements the simplest possible fix.

ACKs for top commit:
  theStack:
    Code-review ACK 937fd4a66f048780bffc5e714d0c800de987ce93
  meshcollider:
    Code review ACK 937fd4a66f048780bffc5e714d0c800de987ce93

Tree-SHA512: 53e6265ed6c7abb47d2b3e77d1604edfeb993c3a2440f0c19679cfeb23516965e6707ff486196a0acfbeff21c79a9a08b5cd33bae9a232d33d0134bca1bd0ff3
2024-07-16 00:25:06 +07:00
MarcoFalke
2f7814acdd
Merge #21035: Remove pointer cast in CRPCTable::dumpArgMap
9048c58e10841d9e1d709c0a325dd14684cec325 Remove pointer cast in CRPCTable::dumpArgMap (Russell Yanofsky)
14f3d9b908ed9e78997bfaad3d8a06357a89d46e refactor: Add RPC server ExecuteCommands function (Russell Yanofsky)
6158a6d3978a18d5ac4166ca2f59056d8ef71c3d refactor: Replace JSONRPCRequest fHelp field with mode field (Russell Yanofsky)

Pull request description:

  This change is needed to fix the `rpc_help.py` test failing in #10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275

  The [`CRPCTable::dumpArgMap`](16b784d953/src/rpc/server.cpp (L492)) method currently works by casting RPC `unique_id` integer field to a function pointer, and then calling it. The `unique_id` field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases) and as a result, this code segfaults in the `rpc_help.py` test in multiprocess PR #10102 because wallet RPC functions aren't directly accessible from the node process.

  Fix this by adding a new `GET_ARGS` RPC request mode to retrieve argument information similar to the way the `GET_HELP` mode retrieves help information.

  ---

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

ACKs for top commit:
  MarcoFalke:
    re-ACK 9048c58e10841d9e1d709c0a325dd14684cec325 👑

Tree-SHA512: cd1a01c1daa5bde2c2455b63548371ee4cf39688313969ad2016d9a0fd4344102e3fd43034058f253364518e9632d57cf21abffad0d6a2c0c94b7a6921cbe615
2024-07-16 00:25:06 +07:00
Konstantin Akimov
d3b1ef374c
refactor: simplify implementation of RPC composite commands 2024-07-16 00:25:06 +07:00
Konstantin Akimov
3270becc9b
chore: add TODO to make client parsing for composite commands 2024-07-16 00:14:15 +07:00
Wladimir J. van der Laan
d55759fa79
Merge #20012: rpc: Remove duplicate name and argNames from CRPCCommand
fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204 rpc: Remove duplicate name and argNames from CRPCCommand (MarcoFalke)
fa92912b4bb4629addcbfdfb7cc000be701614af rpc: Use RPCHelpMan for check-rpc-mappings linter (MarcoFalke)
faf835680be39811827504f77005b6603165f53e rpc: [refactor] Use concise C++11 code in CRPCConvertTable constructor (MarcoFalke)

Pull request description:

  Currently, the RPC argument names are specified twice to simplify consistency linting. To avoid having to specify the argnames twice when adding new arguments, remove the linter and add an equivalent test based on RPCHelpMan.

ACKs for top commit:
  laanwj:
    ACK fa04f9b4ddffc5ef23c2ee7f3cc72a7c2ae49204

Tree-SHA512: 3f5f32f5a09b22d879f24aa67031639d2612cff481d6aebc6cfe6fd757cafb3e7bf72120b30466f59292a260747b71e57322c189d5478b668519b9f32fcde31a
2024-07-16 00:14:14 +07:00
pasta
db828177bf
Merge #6106: feat: create new composite quorum-command platformsign
2db69d7b81 chore: add release notes for "quorum platformsign" (Konstantin Akimov)
283c5f89a2 feat: create new composite command "quorum platformsign" (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  It splits from #6100
  With just whitelist it is impossible to limit the RPC `quorum sign` to use only one specific quorum type, this PR aim to provide ability for quorum signing for platform quorum only.

  ## What was done?
  Implemented a new composite command "quorum platformsign"

  This composite command let to limit quorum type for signing for case of whitelist.
  After that old way to limit platform commands can be deprecated - #6105

  ## How Has This Been Tested?
  Updated a functional tests to use platform signing for Asset Unlocks feature.

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 2db69d7b81
  PastaPastaPasta:
    utACK 2db69d7b81

Tree-SHA512: b0dff9934137c4faa85664058e1e77f85067cc8d931e6d76ee5b9e610164ac8b0609736d5f09475256cb78d65bf92466624d784f0b13d20136df7e75613662cb
2024-07-15 11:52:17 -05:00
pasta
a45e6df58b
Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist
60e36b786a fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  This default doesn't actually exist in code.

  ## What was done?
  Remove default from text

  ## How Has This Been Tested?

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 60e36b786a

Tree-SHA512: 658eb2cf101d0450619461b7ffe6069de5c04c1fdcca216713f9374cc1e60413ec9b96c3a2931fb69a4c2f8277dd6677100270960a58197da3b92dffb1e9e327
2024-07-15 11:52:08 -05:00
pasta
7330982631
Merge #6100: feat: make whitelist works with composite commands for platform needs
85abbb97b4 chore: add release notes for composite command for whitelist (Konstantin Akimov)
78ad778bb0 feat: test composite commands in functional test for whitelist (Konstantin Akimov)
a102a59787 feat: add support of composite commands in RPC'c whitelists (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  https://github.com/dashpay/dash-issues/issues/66
  https://github.com/dashpay/dash-issues/issues/65

  ## What was done?
  Our composite commands such as "quorum list" have been refactored to make them truly compatible with other features, such as whitelist, see https://github.com/dashpay/dash/pull/6052 https://github.com/dashpay/dash/pull/6051 https://github.com/dashpay/dash/pull/6055 and other related PRs

  This PR makes whitelist feature to be compatible with composite commands.

  Instead implementing additional users such "dapi" better to provide universal way which do not require new build for every new API that has been used by platform, let's simplify things.

  Platform at their side can use config such as this one (created based on shumkov's example):
  ```
  rpc: {
            host: '127.0.0.1',
            port: 9998,
            users: [
              {
                user: 'dashmate',
                password: 'rpcpassword',
                whitelist: null,
                lowPriority: false,
              },
              {
                username: 'platform-dapi',
                password: 'rpcpassword',
                whitelist: [],
                lowPriority: true,
              },
              {
                username: 'platform-drive-consensus',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
                lowPriority: false,
              },
              {
                username: 'platform-drive-other',
                password: 'rpcpassword',
                whitelist: [getbestchainlock,getblockchaininfo,getrawtransaction,submitchainlock,verifychainlock,protx_listdiff,quorum_listextended,quorum_info,getassetunlockstatuses,sendrawtransaction,mnsync_status]
  ],
                lowPriority: true,
              },
            ],
            allowIps: ['127.0.0.1', '172.16.0.0/12', '192.168.0.0/16'],
          },
  ```

  ## How Has This Been Tested?
  Updated functional tests, see commits

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    LGTM, utACK 85abbb97b4
  PastaPastaPasta:
    utACK 85abbb97b4

Tree-SHA512: 88608179c347420269880c352cf9f3b46272f3fc62e8e7158042e53ad69dc460d5210a1f89e1e09081d090250c87fcececade88e2ddec09f73f1175836d7867b
2024-07-15 11:51:59 -05:00
pasta
9998ffd92b
Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code
0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 refactor: replace static private member method to static method (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.

  This PR aims to fix this issue, as requested by shumkov for needs of platform:

  > mailformed signature and can’t verify signature due to unknown quorum is the same error?
  > possible to distingush ?

  ## What was done?
  Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
  The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`

  ## How Has This Been Tested?
  Functional test `feature_llmq_chainlocks.py` is updated

  ## Breaking Changes
  `submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 0133c9866d
  PastaPastaPasta:
    utACK 0133c9866d

Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
2024-07-15 11:51:48 -05:00