Commit graph

34711 commits

Author SHA1 Message Date
MacroFake
fa475e9c79
refactor: Return BResult from restoreWallet 2022-07-12 19:20:01 +02:00
MacroFake
fa8de09edc
Prepare BResult for non-copyable types 2022-07-12 19:19:49 +02:00
MacroFake
faace13b71
test: Remove duplicate MAX_BIP125_RBF_SEQUENCE constant 2022-07-12 18:51:18 +02:00
MacroFake
fa0404dbb7
scripted-diff: [test] Rename BIP125_SEQUENCE_NUMBER to MAX_BIP125_RBF_SEQUENCE
-BEGIN VERIFY SCRIPT-
 sed -i 's:BIP125_SEQUENCE_NUMBER:MAX_BIP125_RBF_SEQUENCE:g' $(git grep -l BIP125_SEQUENCE_NUMBER ./test)
-END VERIFY SCRIPT-
2022-07-12 18:49:08 +02:00
MacroFake
1d89fc695a
Merge bitcoin/bitcoin#25324: refactor: add most of src/util to iwyu
07f2c25d04 refactor: add most of src/util to iwyu (fanquake)

Pull request description:

  These files change infrequently, and not much header shuffling is required.

  We don't add everything in src/util/ yet, because IWYU makes some
  dubious suggestions, which I'm going to follow up with upstream.

  Soon we'll swap `src/util/xyz.cpp` for just `src/util/`.

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

Tree-SHA512: 07d76435c2bff1a62c4967eb0efaafe619cc3bbaf4166741d8520927b24336c01aee59822f8082ee2a01e15046a0f5d506b4b23a6e40ceb750f3226ed8167847
2022-07-12 18:05:59 +02:00
MacroFake
46fcb52cb1
Merge bitcoin/bitcoin#24944: rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage
2ef5294a5b rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff test: add getblockfrompeer coverage of invalid inputs (Jon Atack)

Pull request description:

  The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.

  Fix all issues.

ACKs for top commit:
  brunoerg:
    ACK 2ef5294a5b

Tree-SHA512: 454782cf6a44fd0e05483bb152153667ef5c8021358385ddcf89724fbbbd35e187362bdff757e00c99319527bc4c0b20c7187f67241d4585d767a29787142f25
2022-07-12 17:28:26 +02:00
MacroFake
7d258ee8bc
Merge bitcoin/bitcoin#25489: wallet: change ScanForWalletTransactions to use Ticks(Dur2 d)
c6c35db057 wallet: change `ScanForWalletTransactions` to use `Ticks()` (w0xlt)

Pull request description:

  This PR changes `ScanForWalletTransactions()` to use the `Ticks(Dur2 d)` function (introduced in #25456).

ACKs for top commit:
  MarcoFalke:
    cr ACK c6c35db057

Tree-SHA512: 864e136b470baf22293dc03ae3400bbb34955389a1efc83862f006cfac84da9128c3a201ef051606c06f782a1fde84129261dd4b417cbfff854d5c359a92703e
2022-07-12 17:20:12 +02:00
MacroFake
01ae8d9cd2
Merge bitcoin/bitcoin#25592: test persistence of non-mempool tx prioritisation
a9790ba95f [test] persist prioritisation of transactions not in mempool (glozow)

Pull request description:

  We persist tx prioritisation/fee deltas in mempool.dat (see `DumpMempool`). It seems we have test coverage for persistence of modified fees of mempool entries (see `vinfo` loop), but not for the prioritisation of transactions not in mempool (see `mapDeltas`).

  Relevant: https://github.com/bitcoin/bitcoin/pull/25487#discussion_r917490221

ACKs for top commit:
  darosior:
    utACK a9790ba95f
  w0xlt:
    ACK a9790ba95f

Tree-SHA512: 3f2769a917041f12414584f69b2239eef57586f9975869e826f356633fcaf893fde15500619b302e7663de14f3661c6cba22c7524cab5286e715e2c105726521
2022-07-12 17:08:36 +02:00
MacroFake
dd13d7bf16
Merge bitcoin/bitcoin#25591: move-only: Version handshake to libtest_util
fa4be8e7c3 move-only: InitializeNode to handshake helper (MacroFake)
fa7098947c move-only: Version handshake to libtest_util (MacroFake)

Pull request description:

  The version handshake after setting up a peer is an integral part of (unit) testing net processing logic.

  Thus, make the helper accessible in libtest_util.

  Also, remove the peerman argument from `FillNode`, as it must be equal to connman's peerman, which can then be used instead.

ACKs for top commit:
  dergoegge:
    ACK fa4be8e7c3

Tree-SHA512: 8296399dc2c29196bd56584c9b61f1c5a088f96dd3438b07b84e1acf525d867f1e37fdfdeede8a831add25848cda0c221ce3fb873e5ae5ca805a1765aa08eb12
2022-07-12 17:05:42 +02:00
glozow
39d111aee7
Merge bitcoin/bitcoin#25575: Address comments remaining from #25353
1056bbdfcd Address comments remaining from #25353 (Antoine Riard)

Pull request description:

  This PR should address the remaining comments from #25353.

ACKs for top commit:
  MarcoFalke:
    cr ACK 1056bbdfcd
  glozow:
    ACK 1056bbdfcd
  w0xlt:
    cr ACK 1056bbdfcd

Tree-SHA512: 194524193b1f087742c04d3cbe221e2ccf62e1f9303dc6668d62b73bd2dc0c039b7d68b33658dbee7809bd14bb8a5479f8e7928180b18c3180fdfbe3876c3ca1
2022-07-12 15:58:39 +01:00
w0xlt
c6c35db057 wallet: change ScanForWalletTransactions to use Ticks() 2022-07-12 10:29:08 -03:00
MacroFake
316afb1eca
Merge bitcoin/bitcoin#25218: refactor: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination
111ea3ab71 wallet: refactor GetNewDestination, use BResult (furszy)
22351725bc send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy)
198fcca162 wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy)
7a45c33d1f Introduce generic 'Result' class (furszy)

Pull request description:

  Based on a common function signature pattern that we have all around the sources:
  ```cpp
  bool doSomething(arg1, arg2, arg3, arg4, &result_obj, &error_string) {
      // do something...
      if (error) {
          error_string = "something bad happened";
          return false;
      }

      result = goodResult;
      return true;
  }
  ```

  Introduced a generic class `BResult` that encapsulate the function boolean result, the result object (in case of having it) and, in case of failure, the string error reason.

  Obtaining in this way cleaner function signatures and removing boilerplate code:

  ```cpp
  BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
      // do something...
      if (error) return "something bad happened";

      return goodResult;
  }
  ```

  Same cleanup applies equally to the function callers' side as well. There is no longer need to add the error string and the result object declarations before calling the function:

  Before:
  ```cpp
  Obj result_obj;
  std::string error_string;
  if (!doSomething(arg1, arg2, arg3, arg4, result_obj, error_string)) {
      LogPrintf("Error: %s", error_string);
  }
  return result_obj;
  ```

  Now:
  ```cpp
  BResult<Obj> op_res = doSomething(arg1, arg2, arg3, arg4);
  if (!op_res) {
      LogPrintf("Error: %s", op_res.GetError());
  }
  return op_res.GetObjResult();
  ```

  ### Initial Implementation:

  Have connected this new concept to two different flows for now:

  1) The `CreateTransaction` flow. --> 7ba2b87c
  2) The `GetNewDestination` flow. --> bcee0912

  Happy note: even when introduced a new class into the sources, the amount of lines removed is almost equal to added ones :).

  Extra note: this work is an extended version (and a decoupling) of the work that is inside #24845 (which does not contain the `GetNewDestination` changes nor the inclusion of the `FeeCalculation` field inside `CreatedTransactionResult`).

ACKs for top commit:
  achow101:
    ACK 111ea3ab71
  w0xlt:
    reACK 111ea3ab71
  theStack:
    re-ACK 111ea3ab71
  MarcoFalke:
    review ACK 111ea3ab71 🎏

Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69
2022-07-12 13:56:48 +02:00
glozow
a9790ba95f
[test] persist prioritisation of transactions not in mempool 2022-07-12 10:37:13 +01:00
Hennadii Stepanov
27a4dd055b
Merge bitcoin-core/gui#627: Apply translator comments to reset options confirmation dialog
d5c141f221 qt: apply translator comments to reset options confirmation dialog (Jarol Rodriguez)

Pull request description:

  This is a followup to #617. Because the strings were being concatenated, we can not apply translator comments to all of the revelant strings. This can be tested by applying the following diff to current master and running `make translate`; then check the resulting diff:

  ```diff
  diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
  index 462b923d6..3cf165004 100644
  --- a/src/qt/optionsdialog.cpp
  +++ b/src/qt/optionsdialog.cpp
  @@ -286,9 +286,17 @@ void OptionsDialog::on_resetButton_clicked()
   {
       if (model) {
           // confirmation dialog
  +        //: Window title text of pop-up window shown when the user has chosen to reset options.
           QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm options reset"),
  +            /*: Text explaining that the settings the user changed will not come
  +                into effect until the client is restarted. */
               tr("Client restart required to activate changes.") + "<br><br>" +
  +            /*: Text explaining to the user that the client's current settings
  +                will be backed up at a specific location. %1 is a stand-in
  +                argument for the backup location's path. */
               tr("Current settings will be backed up at \"%1\".").arg(m_client_model->dataDir()) + "<br><br>" +
  +            /*: Text asking the user to confirm if they would like to proceed
  +                with a client shutdown. */
               tr("Client will be shut down. Do you want to proceed?"),
               QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);
  ```

  To apply the above translator comments, what we want to do instead is have a variable in which the translatable strings are appended to using the [QString append function](https://doc.qt.io/qt-5/qstring.html#append).

  When you run `make translate` with this PR, you will see the translator comments properly applied, as shown below:
  ``` diff
  diff --git a/src/qt/locale/bitcoin_en.ts b/src/qt/locale/bitcoin_en.ts
  index 35d820187..9e5158b3e 100644
  --- a/src/qt/locale/bitcoin_en.ts
  +++ b/src/qt/locale/bitcoin_en.ts
  @@ -1942,28 +1942,37 @@ Signing is only possible with addresses of the type &apos;legacy&apos;.</source>
           <translation>default</translation>
       </message>
       <message>
  -        <location line="+81"/>
  +        <location line="+86"/>
           <source>none</source>
           <translation type="unfinished"></translation>
       </message>
       <message>
  -        <location line="+97"/>
  +        <location line="+107"/>
           <source>Confirm options reset</source>
  +        <extracomment>Window title text of pop-up window shown when the user has chosen to reset options.</extracomment>
           <translation>Confirm options reset</translation>
       </message>
       <message>
  -        <location line="+1"/>
  -        <location line="+70"/>
  +        <location line="-9"/>
  +        <location line="+79"/>
           <source>Client restart required to activate changes.</source>
  +        <extracomment>Text explaining that the settings changed will not come into effect until the client is restarted.</extracomment>
  +        <translation type="unfinished"></translation>
  +    </message>
  +    <message>
  +        <location line="-75"/>
  +        <source>Current settings will be backed up at &quot;%1&quot;.</source>
  +        <extracomment>Text explaining to the user that the client&apos;s current settings will be backed up at a specific location. %1 is a stand-in argument for the backup location&apos;s path.</extracomment>
           <translation type="unfinished"></translation>
       </message>
       <message>
  -        <location line="-70"/>
  +        <location line="+3"/>
           <source>Client will be shut down. Do you want to proceed?</source>
  +        <extracomment>Text asking the user to confirm if they would like to proceed with a client shutdown.</extracomment>
           <translation type="unfinished"></translation>
       </message>
       <message>
  -        <location line="+18"/>
  +        <location line="+20"/>
           <source>Configuration options</source>
           <extracomment>Window title text of pop-up box that allows opening up of configuration file.</extracomment>
           <translation type="unfinished"></translation>
  ```

  No difference in rendering between master and PR

  | master | PR |
  | ------- | --- |
  <img width="532" alt="Screen Shot 2022-06-29 at 11 39 17 PM" src="https://user-images.githubusercontent.com/23396902/176588495-9d3761b6-9d96-489a-bbe5-a8907f7d5f99.png"> | <img width="532" alt="Screen Shot 2022-06-29 at 11 39 51 PM" src="https://user-images.githubusercontent.com/23396902/176588513-92e29564-b74a-46f5-a5dd-469c4ee953f7.png"> |

ACKs for top commit:
  shaavan:
    ACK d5c141f221
  furszy:
    Tested ACK d5c141f2, no functional changes.
  w0xlt:
    tACK d5c141f221

Tree-SHA512: 6175a096c6f99edb3041cc2429e1ea0670a10cd2cab0364f664a56c7dee1aa8631d52c0a36edb5d571f6ef934e947d45017e446cea7dddae044085c39c8835ef
2022-07-12 08:46:02 +01:00
MacroFake
8479ed02d7
Merge bitcoin/bitcoin#25565: doc: improve developer-notes about threads
dc02edcba1 doc: update the URLs to thread functions in developer-notes (Vasil Dimov)
c5cc3f140c doc: list the I2P accept thread in developer-notes (Vasil Dimov)

Pull request description:

  Document `i2paccept` in `doc/developer-notes.md` and fix broken URLs to doxygen.bitcoincore.org.

ACKs for top commit:
  theStack:
    re-ACK dc02edcba1

Tree-SHA512: 7d396885dd2e8fda2b050aaa25a82b4217ced6a5aa3478339fb892d5392d2b8b6b5997f8bb9acaab7867c0c5bf58bd0b720ef36b335b1e7eb617b8fc205915b0
2022-07-12 09:02:11 +02:00
MacroFake
fa4be8e7c3
move-only: InitializeNode to handshake helper 2022-07-12 08:41:34 +02:00
MacroFake
fa7098947c
move-only: Version handshake to libtest_util 2022-07-12 08:41:24 +02:00
MacroFake
6c0c2620c2
Merge bitcoin/bitcoin#25589: test: speedup wallet_coinbase_category.py
76a84c0a6c test: speedup wallet_coinbase_category.py (furszy)

Pull request description:

  No need to create a chain (200 extra blocks), nor use the cache, for it.

Top commit has no ACKs.

Tree-SHA512: beec64ba6c580d475e19700371ae155f4f3d74325879802da83d02f4153a752d5829b8a4ed77e0c893e79cbc26f66389eed90ac2e8b03a59f6c630ee9333355c
2022-07-12 08:04:54 +02:00
MacroFake
7ba0850c49
Merge bitcoin/bitcoin#25036: wallet: Save wallet scan progress
230a2f4cc3 wallet test: Add unit test for wallet scan save_progress option (Ryan Ofsky)
a89ddfbe22 wallet: Save wallet scan progress (w0xlt)

Pull request description:

  Currently, the wallet scan progress is not saved.
  If it is interrupted,  it will be necessary to start from scratch on the next load.
  This PR changes this and the progress is saved right after checking a block.

  Close https://github.com/bitcoin/bitcoin/issues/25010

ACKs for top commit:
  furszy:
    re-ACK 230a2f4
  achow101:
    ACK 230a2f4cc3
  ryanofsky:
    Code review ACK 230a2f4cc3. Only change since last review is tweaking whitespace and adding log print

Tree-SHA512: 1a9dec207ed22b3443fb06a4daf967637bc02bcaf71c070b7dc33605d0cab959551e4014c9e92293a63f54c5cbcc98bb9f8844a8c60bc32a1482b1c4130fab32
2022-07-12 08:02:22 +02:00
Antoine Riard
1056bbdfcd Address comments remaining from #25353 2022-07-11 18:48:26 -04:00
furszy
76a84c0a6c
test: speedup wallet_coinbase_category.py
No need to create a chain for it (nor use the cache).
2022-07-11 15:13:32 -03:00
Andrew Chow
c92eb6cda0
Merge bitcoin/bitcoin#25562: test: add tests for negative waste during coin selection
98ea43d5e9 test: add tests for negative waste during coin selection (ishaanam)

Pull request description:

  #25495 mentions that waste can be negative when the current feerate is less than the long term feerate. There are currently no waste tests for negative waste, so this PR adds two of them.

ACKs for top commit:
  achow101:
    ACK 98ea43d5e9
  glozow:
    light code review ACK 98ea43d5e9, good to have tests for negative waste

Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
2022-07-11 13:11:25 -04:00
MacroFake
0817cc379f
Merge bitcoin/bitcoin#25512: test: remove wallet dependency and refactor rpc_signrawtransaction.py
0ee43d13e9 test: refactor rpc_signrawtransaction.py (Ayush Sharma)

Pull request description:

  `rpc_signrawtransaction.py` currently tests the `signrawtransactionwithkey` and `signrawtransactionwithwallet` RPCs.

  This PR splits `rpc_signrawtransaction.py` into

  1. `rpc_signrawtransactionwithkey.py`: the tests for `signrawtransactionwithkey` are moved here and this test can now be run with the wallet disabled.
  2.  `wallet_signrawtransactionwithwallet.py`: wallet only tests for `signrawtransactionwithwallet.py`

ACKs for top commit:
  aureleoules:
    tACK 0ee43d13e9.

Tree-SHA512: c7bd2ea746345c978eae231a781fc52953b9d277eb9f8abb9c3270fe1d9f678f23f3784377d7303a2aa23d7ad90097245e517d386b27b4e0016585dfddcb9d49
2022-07-11 15:33:18 +02:00
Vasil Dimov
dc02edcba1
doc: update the URLs to thread functions in developer-notes
ThreadMapPort() does not appear on doxygen.bitcoincore.org
because it is inside `#ifdef`.
2022-07-11 09:56:24 +02:00
MacroFake
327b7e9236
Merge bitcoin/bitcoin#25581: test: refactor: pass absolute fee in create_lots_of_big_transactions helper
6cbe65c5d7 test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper (Sebastian Falbesoner)

Pull request description:

  Recently merged PR #25522 (commit 2222842ae7) enabled specifying an absolute fee for MiniWallet's `create_self_transfer` method. We can use that in the `create_lots_of_big_transactions` helper to avoid deducting the fee manually (with prior conversion from BTC to Satoshis). This helper is used (directly or indirectly) in the tests `feature_maxuploadtarget.py`, `mempool_limit.py`, `mining_prioritisetransaction.py`.

ACKs for top commit:
  MarcoFalke:
    cr ACK 6cbe65c5d7

Tree-SHA512: 63d66939ae36722a2dc787cbd8f1f995de6232139c2169a3d25525f43c7aaacf646d86b4095a8078f26db18e916778c8097acb19ef17ab0f58382b8bb718d60b
2022-07-11 09:21:36 +02:00
Hennadii Stepanov
f9783b0f07
Merge bitcoin-core/gui#471: Add Wallet Restore in the GUI
bc13ec888c doc: Add a release note about the "restore wallet" menu item (w0xlt)
e7a3f698b5 gui: Add Wallet Restore in the GUI (w0xlt)

Pull request description:

  This PR adds a menu item to restore a wallet from a backup file in the GUI.
  Currently this option exists only in RPC interface.

  Motivation: It makes easier for non-technical users to restore backups.

  Master | PR |
  --- | ---
  <img width="307" alt="master" src="https://user-images.githubusercontent.com/94266259/141673349-0bf8a237-ecec-42e4-a0d7-1d5863940036.png"> | <img width="307" alt="pr" src="https://user-images.githubusercontent.com/94266259/141673350-972dea23-ae56-4283-a365-819da62b7067.png"> |

ACKs for top commit:
  w0xlt:
    Added a release note in a new commit (bc13ec888c) to not invalidate the ACKs for the previous one.
  furszy:
    utACK bc13ec8
  shaavan:
    ACK bc13ec888c
  hebasto:
    ACK bc13ec888c

Tree-SHA512: edc3675484238857b77e74382a4041dd5d2cbcda1e2d5bfe52c83d9d7bb7be8a243ecd97e25e994d8c30ab6d7c59ead5a1c953a46dce173666b137eeffc3c94f
2022-07-10 20:43:54 +01:00
Sebastian Falbesoner
6cbe65c5d7 test: refactor: pass absolute fee in create_lots_of_big_transactions helper 2022-07-10 13:09:51 +02:00
Andrew Chow
194710d8ff
Merge bitcoin/bitcoin#25481: wallet: unify max signature logic
d54c5c8b1b wallet: use CCoinControl to estimate signature size (S3RK)
a94659c84e wallet: replace GetTxSpendSize with CalculateMaximumSignedInputSize (S3RK)

Pull request description:

  Currently `DummySignTx` and `DummySignInput` use different ways to determine signature size.
  This PR unifies the way wallet estimates signature size for various inputs.
  Instead of passing boolean flags from calling code the `use_max_sig` is now calculated at the place of signature creation using information available in `CCoinControl`

ACKs for top commit:
  achow101:
    ACK d54c5c8b1b
  theStack:
    Code-review ACK d54c5c8b1b

Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
2022-07-08 10:27:06 -04:00
furszy
111ea3ab71
wallet: refactor GetNewDestination, use BResult 2022-07-08 11:18:35 -03:00
furszy
22351725bc
send: refactor CreateTransaction flow to return a BResult<CTransactionRef> 2022-07-08 11:18:35 -03:00
furszy
198fcca162
wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' 2022-07-08 11:18:35 -03:00
furszy
7a45c33d1f
Introduce generic 'Result' class
Useful to encapsulate the function result object (in case of having it) or, in case of failure, the failure reason.

This let us clean lot of boilerplate code, as now instead of returning a boolean and having to add a ref arg for the
return object and another ref for the error string. We can simply return a 'BResult<Obj>'.

Example of what we currently have:
```
bool doSomething(arg1, arg2, arg3, arg4, &result, &error_string) {
    do something...
    if (error) {
        error_string = "something bad happened";
        return false;
    }

    result = goodResult;
    return true;
}
```

Example of what we will get with this commit:
```
BResult<Obj> doSomething(arg1, arg2, arg3, arg4) {
    do something...
    if (error) return {"something happened"};

    // good
    return {goodResult};
}
```

This allows a similar boilerplate cleanup on the function callers side as well. They don't have to add the extra
pre-function-call error string and result object declarations to pass the references to the function.
2022-07-08 11:18:35 -03:00
Andrew Chow
b9f9ed4640
Merge bitcoin/bitcoin#25337: refactor: encapsulate wallet's address book access
d69045e291 test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy)
324f00a642 refactor: 'ListReceived' use optional for filtered address (furszy)
b459fc122f refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy)
fa9f2ab8fd refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy)
83e42c4b94 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy)
2b48642499 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy)
032842ae41 wallet: implement ForEachAddrBookEntry method (furszy)
09649bc95d refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy)
192eb1e61c refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy)

Pull request description:

  ### Context

  The wallet's `m_address_book` field is being accessed directly from several places across the sources.

  ### Problem

  Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own.

  ### Solution

  Encapsulate `m_address_book` access inside the wallet.

  -------------------------------------------------------

  Extra Note:

  This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR).

ACKs for top commit:
  achow101:
    ACK d69045e291
  theStack:
    ACK d69045e291 
  w0xlt:
    ACK d69045e291

Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
2022-07-08 10:16:08 -04:00
MacroFake
994573726f
Merge bitcoin/bitcoin#25549: doc: update for NetBSD 9.2, add GUI Build Instructions
d3e9a1c71b doc: update for NetBSD 9.2, add GUI Build Instructions (Jarol Rodriguez)

Pull request description:

  **For reviewer:** as I suppose few have a NetBSD system available, I wrote a [guide](https://gist.github.com/jarolrod/385dc063bb02c90aea0cbe8a147fc418#file-netbsd-vm-setup-guide-md) to setup a VM for testing purposes.

  This attempts to update the NetBSD docs so one can successfully build on the latest release. It also adds instructions to build the GUI.

  Additionally, it includes a note and an example on how one could update the gcc version bundled with NetBSD 9.2 and prior to be able to actually compile. This note can be updated with the release of NetBSD 10, as it will package an acceptable gcc version.

  Master: [render](https://github.com/bitcoin/bitcoin/blob/master/doc/build-netbsd.md)
  PR: [render](d3e9a1c71b/doc/build-netbsd.md)

  Related to #20610, but reworked.

ACKs for top commit:
  aureleoules:
    ACK d3e9a1c71b.
  fanquake:
    ACK d3e9a1c71b

Tree-SHA512: fc3c12689cee886f26782c1d57f3b794ceaedc965a571dd06cfc4a57f90393842ad2124e6dba55a12ac9de9bf63d8e3eb4aa541768f2aa8603248175ce7d1c08
2022-07-08 13:17:44 +02:00
fanquake
07f2c25d04
refactor: add most of src/util to iwyu
These files change infrequently, and not much header shuffling is required.

We don't add everything in src/util/ yet, because IWYU makes some
dubious suggestions, which I'm going to follow up with upstream.
2022-07-08 11:06:01 +01:00
MacroFake
a7f3479ba3
Merge bitcoin/bitcoin#25353: Add a -mempoolfullrbf node setting
4c9666bd73 Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard)
aae66ab43d Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard)
3e27e31727 Introduce `mempoolfullrbf` node setting. (Antoine Riard)

Pull request description:

  This is ready for review.

  Recent discussions among LN devs have brought back on the surface concerns about the security of multi-party funded transactions against pinnings attacks and other mempool-based nuisances. The lack of full-rbf transaction-relay topology connected to miners open the way to cheap and naive DoS against multi-party funded transactions (e.g coinjoins, dual-funded channels, on-chain DLCs, ...) without solutions introducing an overhead cost or centralization vectors afaik . For more details, see [0].

  This PR implements a simple `fullrbf` setting, where the node always allows transaction replacement, ignoring BIP125 opt-in flag. The default value of the setting stays **false**, therefore opt-in replacement is still the default Bitcoin Core replacement policy. Contrary to a previous proposal of mine and listening to feedbacks collected since then [1], I think this new setting simply offers more flexibility in a node transaction-relay policy suiting one's application requirements, without arguing a change of the default behavior.

  I [posted](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-June/020557.html) on the ML to invite operators with a bitcoin application sensitive to full-rbf (e.g dual-funded LN channels service providers) or mempool researchers to join a bootstrapped full-rbf activated peers network for experimentation and learning. If people have strong opinions against the existence of such full-rbf transaction-relay network, I'm proposing to express them on the future thread.

  [0] https://lists.linuxfoundation.org/pipermail/lightning-dev/2021-May/003033.html
  [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-June/019074.html

  Follow-up suggestions :
  - soft-enable opt-in RBF in the wallet : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1154918789
  - p2p discovery and additional outbound connection to full-rbf peers : https://github.com/bitcoin/bitcoin/pull/25353#issuecomment-1156044401
  - match the code between RPC, wallet and mempool about disregard of inherited signaling : #22698

ACKs for top commit:
  instagibbs:
    reACK 4c9666bd73
  glozow:
    ACK 4c9666bd73, a few nits which are non-blocking.
  w0xlt:
    ACK 4c9666bd73

Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
2022-07-08 11:06:24 +02:00
MacroFake
172823e4d5
Merge bitcoin/bitcoin#25568: Remove my key from trusted-keys
8cbeab4d90 Remove my key from trusted-keys (Pieter Wuille)

Pull request description:

ACKs for top commit:
  achow101:
    ACK 8cbeab4d90
  junderw:
    ACK 8cbeab4d90
  kristapsk:
    ACK 8cbeab4d90
  1440000bytes:
    ACK 8cbeab4d90
  Zero-1729:
    ACK 8cbeab4d90

Tree-SHA512: a1c313984a7ed8841bc6f95533eedc8c662003cbf79b060b397679fcb5eafdc27527f8cf9894b58fe382a3d1cfac3ade3ea61003a6514bdb2cb568ef21bb02c2
2022-07-08 08:09:50 +02:00
Pieter Wuille
8cbeab4d90 Remove my key from trusted-keys 2022-07-07 16:53:48 -04:00
Hennadii Stepanov
630c1711b4
refactor: Drop no longer needed util/designator.h 2022-07-07 20:01:01 +01:00
Hennadii Stepanov
88ec5d40dc
build: Increase MS Visual Studio minimum version
Visual Studio 2022 with `/std:c++20` supports designated initializers.
2022-07-07 19:59:48 +01:00
Hennadii Stepanov
555f9dd5d3
rpc, refactor: Add decodepsbt_outputs
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).

Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
2022-07-07 19:59:48 +01:00
Hennadii Stepanov
0c432cbbfa
rpc, refactor: Add decodepsbt_inputs
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).

Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
2022-07-07 19:59:48 +01:00
Hennadii Stepanov
01d95a3964
rpc, refactor: Add getblock_prevout
This change eliminates memory usage spike when compiling with Visual
Studio 2022 (at least in Cirrus CI environment).

Easy to review using
`git diff --color-moved-ws=allow-indentation-change --color-moved=dimmed-zebra`
2022-07-07 19:59:41 +01:00
fanquake
5abbc9afec
Merge bitcoin/bitcoin#24832: index: Verify the block filter hash when reading the filter from disk.
e734228d85 Update GCSFilter benchmarks (Calvin Kim)
aee9a8140b Add GCSFilterDecodeSkipCheck benchmark (Patrick Strateman)
299023c1d9 Add GCSFilterDecode and GCSBlockFilterGetHash benchmarks. (Patrick Strateman)
b0a53d50d9 Make sanity check in GCSFilter constructor optional (Patrick Strateman)

Pull request description:

  This PR picks up the abandoned #19280

  BlockFilterIndex was depending on `GolombRiceDecode()` during the filter decode to sanity check that the filter wasn't corrupt. However, we can check for corruption by ensuring that the encoded blockfilter's hash matches up with the one stored in the index database.

  Benchmarks that were added in #19280 showed that checking the hash is much faster.

  The benchmarks were changed to nanobench and the relevant benchmarks were like below, showing a clear win for the hash check method.

  ```
  |             ns/elem |              elem/s |    err% |        ins/elem |       bra/elem |   miss% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------------:|---------------:|--------:|----------:|:----------
  |              531.40 |        1,881,819.43 |    0.3% |        3,527.01 |         411.00 |    0.2% |      0.01 | `DecodeCheckedGCSFilter`
  |          258,220.50 |            3,872.66 |    0.1% |    2,990,092.00 |     586,706.00 |    1.7% |      0.01 | `DecodeGCSFilter`
  |           13,036.77 |           76,706.09 |    0.3% |       64,238.24 |         513.04 |    0.2% |      0.01 | `BlockFilterGetHash`
  ```

ACKs for top commit:
  mzumsande:
    Code Review ACK e734228d85
  theStack:
    Code-review ACK e734228d85
  stickies-v:
    ACK e734228d85
  ryanofsky:
    Code review ACK e734228d85, with caveat that I mostly paid attention to the main code, not the changes to the benchmark. Only changes since last review were changes to the benchmark code.

Tree-SHA512: 02b86eab7b554e1a57a15b17a4d6d71faa91b556c637b0da29f0c9ee76597a110be8e3b4d0c158d4cab04af0623de18b764837be0ec2a72afcfe1ad9c78a83c6
2022-07-07 17:59:02 +01:00
fanquake
d571cf2d24
Merge bitcoin/bitcoin#25500: refactor: Move inbound eviction logic to its own translation unit
0101d2bc3c [net] Move eviction logic to its own file (dergoegge)
c741d748d4 [net] Move ConnectionType to its own file (Cory Fields)
a3c2707039 [net] Add connection type to NodeEvictionCandidate (dergoegge)
42aa5d5b62 [net] Add NoBan status to NodeEvictionCandidate (dergoegge)

Pull request description:

  This PR splits of the first couple commits from #25268 that move the inbound eviction logic from `net.{h,cpp}` to `eviction.{h,cpp}`.

  Please look at #25268 for motivation and conceptual review.

ACKs for top commit:
  jnewbery:
    utACK 0101d2bc3c
  theuni:
    utACK 0101d2bc3c. I quickly verified with `git --color-moved` that the move-only changes are indeed move-only.

Tree-SHA512: e0c345a698030e049cb22fe281b44503c04403c5be5a3750ca14bfcc603a162ac6bac9a39552472feb57c460102b7ca91430b8ad6268f2efccc49b5e8959331b
2022-07-07 17:54:37 +01:00
MacroFake
a658a02c79
Merge bitcoin/bitcoin#25524: add glozow to trusted-keys
ebe106a754 add glozow to trusted-keys (glozow)

Pull request description:

  For maintaining mempool and policy areas of the codebase, as discussed yesterday's meeting: https://gnusha.org/bitcoin-core-dev/2022-06-30.log

ACKs for top commit:
  Sjors:
    ACK ebe106a754 and congrats!
  achow101:
    ACK ebe106a754
  theuni:
    ACK ebe106a754
  dergoegge:
    ACK ebe106a754
  sipa:
    ACK ebe106a754, though relying on others to verify the PGP key.
  laanwj:
    ACK ebe106a754
  josibake:
    ACK ebe106a754 (i have not personally signed this key, but verified it matches other places glozow has posted her key)
  Xekyo:
    ACK ebe106a754.
  brunoerg:
    ACK ebe106a754
  fanquake:
    ACK ebe106a754
  hebasto:
    ACK ebe106a754, confirming my approval given at the IRC meeting.

Tree-SHA512: 215ff8872ea3fa9ca35b25e74a668e50c2e7be3ff653f8d6fd213ac878c33b90bba9defc59a000c644a9df1b06a826eb5d97a89b3c6cca24b5a87c6ab739b01b
2022-07-07 17:49:15 +02:00
fanquake
c49b0405fb
Merge bitcoin/bitcoin#25558: guix: Make windows cross architecture reproducible
5bff18bce5 guix: patch gcc 10 with pthreads to remap guix store paths (Andrew Chow)

Pull request description:

  The only thing preventing windows from being cross architecture reproducible is a single guix store winpthreads path in the debug symbols. This can be removed by patching libgcc to use `-ffile-prefix-map` so that the debug symbol will be mapped to a fixed `/usr` instead of the guix store path which depends on the building architecture.

  x86_64
  ```
  2e585c4a66e930b5e273e89b8aeddc9c3bd1c8375b19d988a6fff64f0d49edfd  guix-build-5bff18bce5d9/output/dist-archive/bitcoin-5bff18bce5d9.tar.gz
  b9235dc1a8541e840231cfafd0d971bd5e8a3ea7d5331c4d7af9dbfdabc6905b  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/SHA256SUMS.part
  f82d861de60e22fc7dd731bef60a3e4399b5317eb16e41e92ded171490d1a578  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64-debug.zip
  bfd59561c3cfce91b09d05b17cfc67cd70cb78eea39ea863119870260a8dbdec  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64-setup-unsigned.exe
  3d049d98c6add13b0eb4c7adcf0d3ae59d1eab09799292a2c900de0ad067912a  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64-unsigned.tar.gz
  7af4c34c47f349028ec1f4c2edea547bd9fa30d1c67977d482607a9c6bf2ddee  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64.zip
  ```

  arm64
  ```
  2e585c4a66e930b5e273e89b8aeddc9c3bd1c8375b19d988a6fff64f0d49edfd  guix-build-5bff18bce5d9/output/dist-archive/bitcoin-5bff18bce5d9.tar.gz
  b9235dc1a8541e840231cfafd0d971bd5e8a3ea7d5331c4d7af9dbfdabc6905b  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/SHA256SUMS.part
  f82d861de60e22fc7dd731bef60a3e4399b5317eb16e41e92ded171490d1a578  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64-debug.zip
  bfd59561c3cfce91b09d05b17cfc67cd70cb78eea39ea863119870260a8dbdec  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64-setup-unsigned.exe
  3d049d98c6add13b0eb4c7adcf0d3ae59d1eab09799292a2c900de0ad067912a  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64-unsigned.tar.gz
  7af4c34c47f349028ec1f4c2edea547bd9fa30d1c67977d482607a9c6bf2ddee  guix-build-5bff18bce5d9/output/x86_64-w64-mingw32/bitcoin-5bff18bce5d9-win64.zip
  ```

ACKs for top commit:
  fanquake:
    ACK 5bff18bce5
  hebasto:
    ACK 5bff18bce5, I have reviewed the code and it looks OK. Confirming reproducibility for `x86_64` and `arm64` platforms.

Tree-SHA512: 7cc34e6348e4cab847a7b8745179fceced0f37d639cf2ae81748dd73820809ea8f5e049b5b3ce2b912528491967e33fafd56e75aa47714e09b41859091433c5d
2022-07-07 16:36:26 +01:00
MacroFake
8ef096d4f8
Merge bitcoin/bitcoin#25522: test: Remove -acceptnonstdtxn=1 from feature_rbf.py
fad690ba0a test: Remove -acceptnonstdtxn=1 from feature_rbf.py (MacroFake)
fa5059b7df test: Make the scriptPubKey of MiniWallet created txs mutable (MacroFake)
fa29245827 test: Allow setting sequence per input in MiniWallet create_self_transfer_multi (MacroFake)
fac3800d2c test: Allow amount_per_output in MiniWallet create_self_transfer_multi (MacroFake)
2222842ae7 test: Allow absolute fee in MiniWallet create_self_transfer (MacroFake)

Pull request description:

  On the main network, nonstandard transactions are hardly relayed, so it seems odd that one of our policy test requires a policy setting opposite of the norm.

  Surely it is also important to test that nonstandard transactions can be replaced. However, rbf code should not care about the standardness at all. Moreover, I think testing nonstandardness rbf is of lower priority than testing the stuff that actually happens in production.

ACKs for top commit:
  theStack:
    re-ACK fad690ba0a
  jamesob:
    ACK fad690ba0a ([`jamesob/ackr/25522.1.MarcoFalke.test_remove_acceptnonstd`](https://github.com/jamesob/bitcoin/tree/ackr/25522.1.MarcoFalke.test_remove_acceptnonstd))

Tree-SHA512: e0a0c808bccdddf738fb6a84e5e5672d7c341bffd941c4f0c232112bfc68265fa81a2e42ddcab107d586358ffdb3dccc46bb5533d46999fd9ab024169dac0f78
2022-07-07 16:12:12 +02:00
laanwj
5ab6a94276
Merge bitcoin/bitcoin#25552: add glozow builder key
8d869a7bb5 add glozow builder key (glozow)

Pull request description:

  This key was used to sign [v23.0rc2](https://github.com/bitcoin-core/guix.sigs/tree/main/23.0rc2/glozow) and [v23.0](https://github.com/bitcoin-core/guix.sigs/tree/main/23.0/glozow).

  https://keys.openpgp.org/search?q=6B002C6EA3F91B1B0DF0C9BC8F617F1200A6D25C
  https://keyserver.ubuntu.com/pks/lookup?search=6B002C6EA3F91B1B0DF0C9BC8F617F1200A6D25C&fingerprint=on&op=index

ACKs for top commit:
  laanwj:
    ACK 8d869a7bb5
  fanquake:
    ACK 8d869a7bb5

Tree-SHA512: 5d134dfcbc999101f9f1afe0e38788230676d1922cb1292371229dba4545e4349b79da3ed38753d4b81523a3e5a21d8726e17bd58db9134795dfe0b279643731
2022-07-07 15:27:05 +02:00
Vasil Dimov
c5cc3f140c
doc: list the I2P accept thread in developer-notes 2022-07-07 15:07:07 +02:00