aboutsummaryrefslogtreecommitdiff
path: root/src
AgeCommit message (Collapse)Author
2022-07-12Merge bitcoin/bitcoin#25218: refactor: introduce generic 'Result' class and ↵MacroFake
connect it to CreateTransaction and GetNewDestination 111ea3ab711414236f8678566a7884d48619b2d8 wallet: refactor GetNewDestination, use BResult (furszy) 22351725bc4c5eb63ee45f607374bbf2d76e2b8c send: refactor CreateTransaction flow to return a BResult<CTransactionRef> (furszy) 198fcca162f4d2f877feab485e629ff89818ff56 wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult' (furszy) 7a45c33d1f8a758850cf8e7bd6ad508939ba5c0d 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 111ea3ab711414236f8678566a7884d48619b2d8 w0xlt: reACK https://github.com/bitcoin/bitcoin/pull/25218/commits/111ea3ab711414236f8678566a7884d48619b2d8 theStack: re-ACK 111ea3ab711414236f8678566a7884d48619b2d8 MarcoFalke: review ACK 111ea3ab711414236f8678566a7884d48619b2d8 🎏 Tree-SHA512: 6d84d901a4cb923727067f25ff64542a40edd1ea84fdeac092312ac684c34e3688a52ac5eb012717d2b73f4cb742b9d78e458eb0e9cb9d6d72a916395be91f69
2022-07-12Merge bitcoin-core/gui#627: Apply translator comments to reset options ↵Hennadii Stepanov
confirmation dialog d5c141f221d67aaeee989da0db0ac5383d7562d3 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 d5c141f221d67aaeee989da0db0ac5383d7562d3 furszy: Tested ACK d5c141f2, no functional changes. w0xlt: tACK https://github.com/bitcoin-core/gui/pull/627/commits/d5c141f221d67aaeee989da0db0ac5383d7562d3 Tree-SHA512: 6175a096c6f99edb3041cc2429e1ea0670a10cd2cab0364f664a56c7dee1aa8631d52c0a36edb5d571f6ef934e947d45017e446cea7dddae044085c39c8835ef
2022-07-12Merge bitcoin/bitcoin#25036: wallet: Save wallet scan progressMacroFake
230a2f4cc3fab9f66b6c24ba809ddbea77755cb7 wallet test: Add unit test for wallet scan save_progress option (Ryan Ofsky) a89ddfbe22b6db5beda678c9493e08fec6144122 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 230a2f4cc3fab9f66b6c24ba809ddbea77755cb7 ryanofsky: Code review ACK 230a2f4cc3fab9f66b6c24ba809ddbea77755cb7. Only change since last review is tweaking whitespace and adding log print Tree-SHA512: 1a9dec207ed22b3443fb06a4daf967637bc02bcaf71c070b7dc33605d0cab959551e4014c9e92293a63f54c5cbcc98bb9f8844a8c60bc32a1482b1c4130fab32
2022-07-11Merge bitcoin/bitcoin#25562: test: add tests for negative waste during coin ↵Andrew Chow
selection 98ea43d5e9fc7b001f55a5bb1602afc2661cdb0f 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 98ea43d5e9fc7b001f55a5bb1602afc2661cdb0f glozow: light code review ACK 98ea43d5e9fc7b001f55a5bb1602afc2661cdb0f, good to have tests for negative waste Tree-SHA512: d194d370f1257975959d3c601fea9f82c30c1aabc3e8bedc997c62659283fe681cc527e59df1a0187b3c91e8067c60374dd5ce0237561bd882edafe6a575a9b9
2022-07-10Merge bitcoin-core/gui#471: Add Wallet Restore in the GUIHennadii Stepanov
bc13ec888cdc2791f79eeb6eb76b9134d670043e doc: Add a release note about the "restore wallet" menu item (w0xlt) e7a3f698b5f3f7dde8632c4911abd4e5bc1f06cb 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 (https://github.com/bitcoin-core/gui/pull/471/commits/bc13ec888cdc2791f79eeb6eb76b9134d670043e) to not invalidate the ACKs for the previous one. furszy: utACK bc13ec8 shaavan: ACK bc13ec888cdc2791f79eeb6eb76b9134d670043e hebasto: ACK bc13ec888cdc2791f79eeb6eb76b9134d670043e Tree-SHA512: edc3675484238857b77e74382a4041dd5d2cbcda1e2d5bfe52c83d9d7bb7be8a243ecd97e25e994d8c30ab6d7c59ead5a1c953a46dce173666b137eeffc3c94f
2022-07-08Merge bitcoin/bitcoin#25481: wallet: unify max signature logicAndrew Chow
d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9 wallet: use CCoinControl to estimate signature size (S3RK) a94659c84ee10ac5915eb5a6b654435183d88521 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 d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9 theStack: Code-review ACK d54c5c8b1b1a38b5b38e6878aea0fa8d6c1ad7e9 Tree-SHA512: e790903ad4683067070aa7dbf7434a1bd142282a5bc425112e64d88d27559f1a2cd60c68d6022feaf6b845237035cb18ece10f6243d719ba28173b69bd99110a
2022-07-08wallet: refactor GetNewDestination, use BResultfurszy
2022-07-08send: refactor CreateTransaction flow to return a BResult<CTransactionRef>furszy
2022-07-08wallet: refactor, include 'FeeCalculation' inside 'CreatedTransactionResult'furszy
2022-07-08Introduce generic 'Result' classfurszy
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-08Merge bitcoin/bitcoin#25337: refactor: encapsulate wallet's address book accessAndrew Chow
d69045e291e32e02d105d1b5ff1c8b86db0ae69e test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy) 324f00a6420bbd64c67c264e50632e6fa36ae732 refactor: 'ListReceived' use optional for filtered address (furszy) b459fc122feace9e9a738c48aab21961cf15dddc refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy) fa9f2ab8fd53075d2a3ec93ddac4908e73525c46 refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy) 83e42c4b94e376a19d3eb0a2379769b8b8ac5fc8 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy) 2b48642499016cb357e4bcec32481cd50361194e refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy) 032842ae4196aaed5ea3567ea01a61ed75ab2edd wallet: implement ForEachAddrBookEntry method (furszy) 09649bc95d5f2855a54a8cf02e65215a3b333c92 refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy) 192eb1e61c3c43baec7f32c498ab0ce0656a58f7 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 d69045e291e32e02d105d1b5ff1c8b86db0ae69e theStack: ACK d69045e291e32e02d105d1b5ff1c8b86db0ae69e ✅ w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25337/commits/d69045e291e32e02d105d1b5ff1c8b86db0ae69e Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
2022-07-08Merge bitcoin/bitcoin#25353: Add a `-mempoolfullrbf` node settingMacroFake
4c9666bd73645b94ae81be1faf7a0b8237dd6e99 Mention `mempoolfullrbf` in policy/mempool-replacements.md (Antoine Riard) aae66ab43d794bdfaa2dade91760cc55861c9693 Update getmempoolinfo RPC with `mempoolfullrbf` (Antoine Riard) 3e27e317270fdc2dd02794fea9da016387699636 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 https://github.com/bitcoin/bitcoin/pull/25353/commits/4c9666bd73645b94ae81be1faf7a0b8237dd6e99 glozow: ACK 4c9666bd73645b94ae81be1faf7a0b8237dd6e99, a few nits which are non-blocking. w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/25353/commits/4c9666bd73645b94ae81be1faf7a0b8237dd6e99 Tree-SHA512: 9e288bf22e06a9808804e58178444ef1830c3fdd42fd8a7cd7ffb101f8f586e08b000679be407d63ca76a56f7216227b368ff630c81f3fac3243db1a1202ab1c
2022-07-07Merge bitcoin/bitcoin#24832: index: Verify the block filter hash when ↵fanquake
reading the filter from disk. e734228d8585c0870c71ce8ba8c037f8cf8b249a Update GCSFilter benchmarks (Calvin Kim) aee9a8140b3a58b744766f9e89572f1d953a808b Add GCSFilterDecodeSkipCheck benchmark (Patrick Strateman) 299023c1d9962628d158fac0306f8531506a0123 Add GCSFilterDecode and GCSBlockFilterGetHash benchmarks. (Patrick Strateman) b0a53d50d9142bed51a8372eeb848816bfa94da8 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 e734228d8585c0870c71ce8ba8c037f8cf8b249a theStack: Code-review ACK e734228d8585c0870c71ce8ba8c037f8cf8b249a stickies-v: ACK e734228d8585c0870c71ce8ba8c037f8cf8b249a ryanofsky: Code review ACK e734228d8585c0870c71ce8ba8c037f8cf8b249a, 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-07Merge bitcoin/bitcoin#25500: refactor: Move inbound eviction logic to its ↵fanquake
own translation unit 0101d2bc3c3bcf698d6cc2a237a680fc52395987 [net] Move eviction logic to its own file (dergoegge) c741d748d4d9836940b99091cc7be09c65efcb79 [net] Move ConnectionType to its own file (Cory Fields) a3c27070396ab8c2941c437e8099547e8fc9c110 [net] Add connection type to NodeEvictionCandidate (dergoegge) 42aa5d5b6269d27af525d5001907558442e96023 [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 0101d2bc3c3bcf698d6cc2a237a680fc52395987 theuni: utACK 0101d2bc3c3bcf698d6cc2a237a680fc52395987. I quickly verified with `git --color-moved` that the move-only changes are indeed move-only. Tree-SHA512: e0c345a698030e049cb22fe281b44503c04403c5be5a3750ca14bfcc603a162ac6bac9a39552472feb57c460102b7ca91430b8ad6268f2efccc49b5e8959331b
2022-07-07Merge bitcoin/bitcoin#25447: fuzz: add low-level target for txorphanageMacroFake
6eb0909cb7d5883a258f76ad6cf2c989fc6f892f fuzz: add low-level target for txorphanage (chinggg) Pull request description: This adds a low-level fuzz target for orphan transaction handling by creating random transactions and calling all functions in `TxOrphanage`. It cannot simulate real-world `orphan/unorphan` scenarios effectively since it does not maintain any state about the node and the chain. A high-level fuzz target which construct well-designed transaction graphs will be added later. ACKs for top commit: MarcoFalke: review ACK 6eb0909cb7d5883a258f76ad6cf2c989fc6f892f 🐈 Tree-SHA512: b4d64f5941df77d13981f75ec170cef6ffabe782797c982ede7f34134be01dc0026dd7c0bee614bc1d64715e90a933d2a8c95974d402e32eaba8e24cc928299e
2022-07-07test: add tests for negative waste during coin selectionishaanam
2022-07-06Update getmempoolinfo RPC with `mempoolfullrbf`Antoine Riard
2022-07-06Introduce `mempoolfullrbf` node setting.Antoine Riard
This new node policy setting enables to accept replaced-by-fee transaction without inspection of the replaceability signaling as described in BIP125 "explicit signaling". If turns on, the node mempool accepts transaction replacement as described in `policy/mempool-replacements.md`. The default setting value is `false`, implying opt-in RBF is enforced.
2022-07-06[net] Move eviction logic to its own filedergoegge
2022-07-06[net] Move ConnectionType to its own fileCory Fields
2022-07-06Merge bitcoin/bitcoin#25507: wallet: don't add change fee to target if ↵Andrew Chow
subtracting fees from output 140d942634f9f1bba191aafa948df57812c0f3fe wallet: don't add change fee to target if subtracting fees from output (S3RK) Pull request description: Change fee is payed by the recipient, so we don't need to add it to our target for coin selection. ACKs for top commit: achow101: ACK 140d942634f9f1bba191aafa948df57812c0f3fe ishaanam: ACK 140d942634f9f1bba191aafa948df57812c0f3fe furszy: Code review ACK 140d9426 Tree-SHA512: b5efd0264c47ecee9204a3fd039bad24c69f9e614c6e1d9bb240ee5be6356b175aa074f3be123e6cfb8becd4d7bd1028eebe18801662cc69d19413d8d5a9dd5c
2022-07-06fuzz: add low-level target for txorphanagechinggg
2022-07-04Merge bitcoin/bitcoin#25454: p2p: Avoid multiple getheaders messages in ↵fanquake
flight to the same peer 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 Replace GetTime() with NodeClock in MaybeSendGetHeaders() (Suhas Daftuar) abf5d16c24cb08b0451bdbd4d1de63a12930e8f5 Don't send getheaders message when another request is outstanding (Suhas Daftuar) ffe87db247b19ffb8bfba329c5dd0be39ef5a53f Cleanup received_new_header calculation to use WITH_LOCK (Suhas Daftuar) 6d95cd3e7444ebaaabb64a76783ea3551530f1d7 Move peer state updates from headers message into separate function (Suhas Daftuar) 2b341db731793844f12944363186edea23eabdeb Move headers direct fetch to end of ProcessHeadersMessage (Suhas Daftuar) 29c45185223441943ab610e62937a118c7c3a5b2 Move headers-direct-fetch logic into own function (Suhas Daftuar) bf8ea6df75749c27f753b562c4724b3f8d263ad4 Move additional headers fetching to own function (Suhas Daftuar) 9492e93bf9f4a841bf43ca4b593871c0863d5b63 Add helper function for checking header continuity (Suhas Daftuar) 7f2450871b3ea0b4d02d56bd2ca365fcc25cf90e Move handling of unconnecting headers into own function (Suhas Daftuar) Pull request description: Change `getheaders` messages so that we wait up to 2 minutes for a response to a prior `getheaders` message before issuing a new one. Also change the handling of the `getheaders` message sent in response to a block INV, so that we no longer use the hashstop variable (including the hash stop will just mean that if our peer's headers chain is longer, then we won't learn it, so there's no benefit to using hashstop). Also, now respond to a `getheaders` during IBD with an empty headers message (rather than nothing) -- this better conforms to the intent of the new logic that it's better to not ignore a peer's `getheaders` message, even if you have nothing to give. This also avoids a lot of functional tests breaking. This PR also reworks the headers processing logic to make it more readable. ACKs for top commit: ajtowns: ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 ; code review, check over new logic of when to send getheaders messages dergoegge: Code review ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 mzumsande: Code Review ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 sipa: utACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 w0xlt: tACK https://github.com/bitcoin/bitcoin/pull/25454/commits/99f4785cad94657dcf349d00fdd6f1d44cac9bb0 Good improvement in the code. Tree-SHA512: b8a63f6f71ac83e292edc0200def7835ad8b06b2955dd34e3ea6fac85980fa6962efd31d689ef5ea121ff5477ec14aafa4bbe2d0db134c05f4a31a57a8ced365
2022-07-04[net] Add connection type to NodeEvictionCandidatedergoegge
2022-07-04[net] Add NoBan status to NodeEvictionCandidatedergoegge
2022-07-01Merge bitcoin/bitcoin#25521: build: Remove outdated libbitcoinkernel commentMacroFake
18f5355f3ad8a5513c99c5b0ca14266b604362cc Remove outdated comment (Igor Bubelov) Pull request description: Looks like this comment is no longer relevant, the last files which matched `index/*.cpp` pattern were removed in https://github.com/bitcoin/bitcoin/commit/f1006875665ffe8ff5da8185effe25b860743b4e ACKs for top commit: dongcarl: ACK 18f5355f3ad8a5513c99c5b0ca14266b604362cc shaavan: ACK 18f5355f3ad8a5513c99c5b0ca14266b604362cc Tree-SHA512: d3fcc2db0940f81ce521fddff836f271709ea327c357942383f8aff6c7089e74490fa720e7a2916900215c733d2b64960c1aa185f0c9b1567fce90a249d405e0
2022-07-01Merge bitcoin/bitcoin#25471: rpc: Disallow gettxoutsetinfo queries for a ↵MacroFake
specific block with `use_index=false` 27c8056885b05bd25f540dbf6ede89230d43c7b9 rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=false (Martin Zumsande) Pull request description: In the `gettxoutsetinfo` RPC, if we set `use_index` to false but specify `hash_or_height`, we currently hit a nonfatal error, e.g. `gettxoutsetinfo "muhash" "1" "false"` results in: ``` Internal bug detected: "!pindex || pindex->GetBlockHash() == view->GetBestBlock()" rpc/blockchain.cpp:836 (GetUTXOStats) ``` The failing check was added in [#24410](https://github.com/bitcoin/bitcoin/pull/24410/commits/664a14ba7ccb40aa82d35a59831acd35db1897a6), but the previous behavior, returning the specified height together with data corresponding to the tip's height, was very confusing too in my opinion. Fix this by disallowing the interaction of `use_index=false` and `hash_or_height` and add a RPC help example with `-named` because users might ask themselves how to use the `use_index` flag witout hitting an error. An alternative way would be to allow the interaction if the specified `hash_or_height` happens to correspond to the tip (which should then also be applied to the `HASH_SERIALIZED` check before). If reviewers would prefer that, please say so. ACKs for top commit: fjahr: utACK 27c8056885b05bd25f540dbf6ede89230d43c7b9 shaavan: ACK 27c8056885b05bd25f540dbf6ede89230d43c7b9 Tree-SHA512: 1d81c34eaa48c86134a2cf7193246d5de6bfd819d413c3b3fae9cb9290e0297a336111eeaecede2f0f020b0f9a181d240de0da4493e1b387fe63b8189154442b
2022-07-01Replace GetTime() with NodeClock in MaybeSendGetHeaders()Suhas Daftuar
2022-07-01Remove outdated commentIgor Bubelov
2022-06-30Merge bitcoin/bitcoin#25383: wallet: don't read db every time that a new ↵MacroFake
'WalletBatch' is created c318211ddd48d44dd81dded553afeee3bc41c89e walletdb: fix last client version update (furszy) bda8ebe608e6572eaaf40cd28dab6954241c9b0d wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on #25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211ddd48d44dd81dded553afeee3bc41c89e w0xlt: reACK https://github.com/bitcoin/bitcoin/pull/25383/commits/c318211ddd48d44dd81dded553afeee3bc41c89e Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
2022-06-30Merge bitcoin/bitcoin#24836: add RPC (-regtest only) for testing package policyfanquake
e866f0d0666664885d4c15c79bf59cc59975887a [functional test] submitrawpackage RPC (glozow) fa076515b07ac4b10b2134e323bf4f56be5996a8 [rpc] add new submitpackage RPC (glozow) Pull request description: It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist. Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp. ACKs for top commit: t-bast: Tested ACK against eclair https://github.com/bitcoin/bitcoin/pull/24836/commits/e866f0d0666664885d4c15c79bf59cc59975887a ariard: Code Review ACK e866f0d0 instagibbs: code review ACK e866f0d0666664885d4c15c79bf59cc59975887a Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
2022-06-30rephrase error for invalid timeout/dev/fd0
2022-06-30wallet: don't add change fee to target if subtracting fees from outputS3RK
2022-06-29qt: apply translator comments to reset options confirmation dialogJarol Rodriguez
Follow-up to #617. This applies translator strings to the reset options confirmation dialog and also refactors the way we pass the strings to the dialog in order to allow the comments to be applied. Because the strings were being concatenated, we can not apply translator comments to all of the relevant strings. What we want to do instead is have a variable in which the translatable strings are appended to using the QString append function. This satisfies the Qt translator engine and the comments are then properly applied within the `extracomment` field in the translation file.
2022-06-29Merge bitcoin/bitcoin#25502: upstream: update minisketch subtreeAndrew Chow
28a28a0c5bfae1a4149c795676a3109fd614b22a Squashed 'src/minisketch/' changes from 7eeb778fef..47f0a2d26f (fanquake) Pull request description: Contains: * https://github.com/sipa/minisketch/pull/65 * https://github.com/sipa/minisketch/pull/66 Required for #25493. ACKs for top commit: achow101: ACK dc375e5ccee5599374dddc2f0141a44a67c363b8 hebasto: ACK dc375e5ccee5599374dddc2f0141a44a67c363b8, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: fbcd6cdc551770ff67d1df65ab171ce43c9eb7e7668da76da5c5b06865ed550527abcff661741a86c1535018a85a165619ff94ae3e6c7a695374b6c4f843c5ca
2022-06-29Merge bitcoin/bitcoin#25497: wallet: more accurate target for large transactionsAndrew Chow
25e4762ae7a09906020e62cd947e9f71300439cf wallet: more accurate tx_noinputs_size (S3RK) Pull request description: Rationale: more accurate non-input fee estimation for txs with >=253 inputs ACKs for top commit: laanwj: Concept and code review ACK 25e4762ae7a09906020e62cd947e9f71300439cf achow101: ACK 25e4762ae7a09906020e62cd947e9f71300439cf furszy: Code review ACK 25e4762a. left a small nit. Tree-SHA512: bda8fad725d32ad3e13c007fa56ddb6679ac1a32098ddb08d9a114054acfa681cb66cd703ac675297f731cb381b09067a99a4efa31320140bbdd03f0cfdc81af
2022-06-29Update minisketch subtree to latest masterfanquake
2022-06-29Merge bitcoin/bitcoin#25495: Revert "bnb: exit selection when best_waste is 0"fanquake
af56d63eca8a246c02506c2aef7ea8a22e2d07bb Revert "bnb: exit selection when best_waste is 0" (Murch) Pull request description: This reverts commit 9b5950db8683f9b4be03f79ee0aae8a780b01a4b. Waste can be negative. At feerates lower than long_term_feerate this means that a waste of 0 may be a suboptimal solution and this causes the search to exit prematurely. Only when the feerate is equal to the long_term_feerate would achieving a waste of 0 indicate that we have achieved an optimal solution, because it would mean that the excess is 0. It seems unlikely that this would ever occur outside of test cases, and even then we should prefer solutions with more inputs over solutions with fewer according to previous decisions—but solutions with more inputs are found later in the branch exploration. The "optimization" described in #18257 and implemented in #18262 is therefore a premature exit on a suboptimal solution and should be reverted. ACKs for top commit: sipa: utACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb S3RK: utACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb achow101: ACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb glozow: utACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb, agree it is incorrect to stop here unless we could rule out the possibility of a better solution with negative waste. `SelectCoinsBnB` doesn't know what long term feerate and effective feerate are (and probably shouldn't) so it's better to have no exit early condition at all. Tree-SHA512: 470f1a49041a0042cb69d239fccac7512ace79871d43508b6e7f7a2f3aca3523930b16e00c5513b816d5fe078d9ab53e42b0a80fd3c3d48e6434f24c2b009077
2022-06-29util: remove MSVC warning pragmasfanquake
4786 - I don't think this exists any more? 4805 - Is already defined (globally) in the MSVC project. Dropped 4717 and 4804, as it seems they are no-longer supressing anything. See: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-c4000-c5999
2022-06-29Merge bitcoin/bitcoin#25290: [kernel 3a/n] Decouple `CTxMemPool` from ↵MacroFake
`ArgsManager` d1684beabe5b738c2cc83de83e1aaef11a761b69 fees: Pass in a filepath instead of referencing gArgs (Carl Dong) 9a3d825c30e8e6118d74a4e568744cb9d03f7f5d init: Remove redundant -*mempool*, -limit* queries (Carl Dong) 6c5c60c4124293d948735756f84efc85262ea66f mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong) 9e93b1030182eff92ef91181e17c7dd498c7e164 node/ifaces: Use existing MemPoolLimits (Carl Dong) 38af2bcf358a72b9457d370282e57f4be1c5c849 mempoolaccept: Use limits from mempool in constructor (Carl Dong) 9333427014695ac235c96d48791098168dfdc9db mempool: Introduce (still-unused) MemPoolLimits (Carl Dong) 716bb5fbd31077bbe99d11a54d6c2c250afc8085 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong) 1ecc77321deb61b9f6888e4e10752b9d972fd26e scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong) aa9141cd8185cb7ad532bc16feb9d302b05d9697 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong) 51c7a41a5eb6fcb60333812c770d80227cf7b64d init: Only determine maxmempool once (Carl Dong) 386c9472c8764738282e6d163b42e15a8feda7ea mempool: Make GetMinFee() with custom size protected (Carl Dong) 82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong) f1941e8bfd2eecc478c7660434b1ebf6a64095a0 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong) 0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 fuzz/rbf: Add missing TestingSetup (Carl Dong) ccbaf546a68d6cda8ed3efd0598c0e4121b366bb scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong) fc02f77ca604f0221171bfde3059b34f5d0fb1cd ArgsMan: Add Get*Arg functions returning optional (Carl Dong) Pull request description: This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18 ----- As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this. This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time. These options are: - `-maxmempool` -> `CTxMemPool::Options::max_size` - `-mempoolexpiry` -> `CTxMemPool::Options::expiry` - `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count` - `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size` - `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count` - `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size` More context can be gleaned from the commit messages. The important commits are: - 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions" - a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs" - 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs" - 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits" Reviewers: Help needed in the following commits (see commit messages): - a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs" - 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits" Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch. ----- TODO: - [x] Use the more ergonomic `CTxMemPool::Options` where appropriate - [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions` ----- Questions for Reviewers: 1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?) 2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`? ACKs for top commit: MarcoFalke: ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜 ryanofsky: Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
2022-06-29wallet: more accurate tx_noinputs_sizeS3RK
2022-06-29Merge bitcoin-core/gui#617: Reset options, notify user about backup creationHennadii Stepanov
ac4fb3bbbe207d0744201a9df8a971af06db5476 gui: reset options, notify user about the backup creation (furszy) Pull request description: Quick follow-up to first point of https://github.com/bitcoin-core/gui/pull/602#pullrequestreview-1002780997 ACKs for top commit: ryanofsky: Code review ACK ac4fb3bbbe207d0744201a9df8a971af06db5476, just fixing displayed backup directory since last review jarolrod: tACK ac4fb3bbbe207d0744201a9df8a971af06db5476 Tree-SHA512: cfeca5cd6d6d3d69bbd81211cf1bfd490de13ac96bf53be081a5ceb88611afa57dff2be35f8e0a41b1088b7b892f75a21a9abf47f2e1d77e9e316467eb7c12be
2022-06-28rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=falseMartin Zumsande
by returning an RPC error where previously a NonFatalError would be thrown.
2022-06-28Revert "bnb: exit selection when best_waste is 0"Murch
This reverts commit 9b5950db8683f9b4be03f79ee0aae8a780b01a4b. Waste can be negative. At feerates lower than long_term_feerate this means that a waste of 0 may be a suboptimal solution and this causes the search to exit prematurely. Only when the feerate is equal to the long_term_feerate would achieving a waste of 0 indicate that we have achieved an optimal solution, because it would mean that the excess is 0. It seems unlikely that this would ever occur outside of test cases, and even then we should prefer solutions with more inputs over solutions with fewer according to previous decisions—but solutions with more inputs are found later in the branch exploration. The "optimization" described in #18257 and implemented in #18262 is therefore a premature exit on a suboptimal solution and should be reverted.
2022-06-28fees: Pass in a filepath instead of referencing gArgsCarl Dong
2022-06-28init: Remove redundant -*mempool*, -limit* queriesCarl Dong
Now that MemPoolOptions has correctly-determined max_size and limits members, perform sanity checks on that instead of re-determining the options.
2022-06-28Don't send getheaders message when another request is outstandingSuhas Daftuar
Change getheaders messages so that we wait up to 2 minutes for a response to a prior getheaders message before issuing a new one. Also change the handling of the getheaders message sent in response to a block INV, so that we no longer use the hashstop variable (including the hash stop will just mean that if our peer's headers chain is longer, then we won't learn it, so there's no benefit to using hashstop). Also, now respond to a getheaders during IBD with an empty headers message (rather than nothing) -- this better conforms to the intent of the new logic that it's better to not ignore a peer's getheaders message, even if you have nothing to give. This also avoids a lot of functional tests breaking. p2p_segwit.py is modified to use this same strategy, as the test logic (of expecting a getheaders after a block inv) would otherwise be broken.
2022-06-28Cleanup received_new_header calculation to use WITH_LOCKSuhas Daftuar
2022-06-28Move peer state updates from headers message into separate functionSuhas Daftuar
2022-06-28Move headers direct fetch to end of ProcessHeadersMessageSuhas Daftuar
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-09 08:51:24 +0000