backport: bitcoin#19801, #20556, #21141, #21235, #21331, #21343, #21424, #21691, #21777 #6114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
changed the title
UdjinM6
requested changes
Jul 15, 2024
|
This pull request has conflicts, please rebase. |
…bitcoin#18592 rebased) 7c90c67 rpc: refactor rpc wallet functions to take references instead of pointers (fanquake) 4866934 rpc: remove calls to CWallet.get() (fanquake) Pull request description: This is a rebased bitcoin#18592. > This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here bitcoin#18590 > It seems that this PR is indirectly related to this issue: bitcoin#13063 (comment) > 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 bitcoin#18590 ACKs for top commit: MarcoFalke: ACK 7c90c67 🐧 ryanofsky: Code review ACK 7c90c67. Changes easy to review with `--word-diff-regex=. -U0` Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
…k, gettxout, getblocktemplate, scantxoutset) fa7ff07 rpc: Properly document submitblock return value (MarcoFalke) fae542c rpc: Properly document getblocktemplate return value (MarcoFalke) fabaccf rpc: Properly document scantxoutset return value (MarcoFalke) faa2059 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 bitcoin#18476 ACKs for top commit: fjahr: utACK fa7ff07 amitiuttarwar: tACK fa7ff07 Tree-SHA512: 933cb8f003163d93dbedb302d4c162514c2698ec6d58dbb9a053da8b8b9a4459b0701a3d9e830ecdabd7f278a46b7a07a3af49ec60703a80fcd75390877294ea
6927933 [net processing] Add ChainSyncTimeoutState default initializers (John Newbery) 55966e0 [net processing] Remove CNodeState ctor body (John Newbery) Pull request description: This addresses the two outstanding review comments from bitcoin#21370. ACKs for top commit: practicalswift: cr ACK 6927933: patch looks correct hebasto: ACK 6927933, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: b3ef5c8a096e447887df255406b3a760f01c73e2b942374595416b4b4031fc69b89cd93168c45040489d581f340b2a62d3fbabd207d4307f587c00a7a7daacd1
…alletnotify 06e1fb0 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 06e1fb0 Tree-SHA512: d2744e2a7a883f9c3a9fd32237110e048c4b6b11fea8221c33d10b74157f65bbc4351211f441e8c1a4af5d5d38e2ba6b1943a7673dc18860c0553d7b41e00775
c180c91 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 bitcoin#20601 and bitcoin#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](https://github.com/bitcoin/bitcoin/blob/c180c911b88b2bd2baf2c9c2b24e276787ffb69b/doc/build-osx.md) ACKs for top commit: fanquake: ACK c180c91 - 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
…tBlockData, remove send bool fa81773 style-only: Remove whitespace (MarcoFalke) fae77b9 net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman) fae7c04 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 bitcoin#13670 ACKs for top commit: jnewbery: utACK fa81773 sipa: utACK fa81773 Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
fa8eaee test: Run versionbits_sanity for all chains (MarcoFalke) faec1e9 test: Address outstanding versionbits_test feedback (MarcoFalke) fad4167 test: Check that no versionbits are re-used (MarcoFalke) Pull request description: ACKs for top commit: jnewbery: Code review ACK fa8eaee ajtowns: ACK fa8eaee code review only Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
…s in feature_cltv.py (BIP 65) b01cd94 test: check that _all_ invalid-CLTV txs are rejected after BIP65 activation (Sebastian Falbesoner) dbc1981 test: check that _all_ invalid-CLTV txs are allowed in a block pre-BIP65 (Sebastian Falbesoner) 8d0ce50 test: prepare cltv_invalidate to test all failure reasons in feature_cltv.py (Sebastian Falbesoner) ce994e1 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: https://github.com/bitcoin/bitcoin/blob/f8462a6d2794be728cf8550f45d19a354aae59cf/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 b01cd94 🐣 Tree-SHA512: dd82ae86e2bc4f3ab9bb1cfc9f04e4431b2b59c8aaf2a9f4b28654a1577e003fb43c500f99d76ff57e96262168e1cad7c1a0d71158e4b01063737e8f4be1e07d
…issue fa4aec2 test: Fix feature_notifications.py intermittent issue (MarcoFalke) Pull request description: Fixes bitcoin#21683 Top commit has no ACKs. Tree-SHA512: 256806d82877477f4b3d795658f61127c0de4eff07216f6071f40a8ec1f5d43f3c587f35dd436d480dc261ef6646ac5547db104d22f3fcfeeb61bbdbe04bcc31
UdjinM6
approved these changes
Jul 19, 2024
knst
added a commit
to knst/dash
that referenced
this pull request
Jul 23, 2024
It happened due to 2 PRs merged one after each other without rebasing: - enabling -Wdocumentation in bitcoin#21631 (dashpay#6113) - renaming param while doxygen comment is forgotten in dashpay#6114
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 23, 2024
…o wallet bebf391 fix: build with -Wdocumentation - rename param pwallet to wallet (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented CI failures like that: https://gitlab.com/dashpay/dash/-/jobs/7400661581 It happened due to 2 PRs merged one after each other without rebasing: - enabling -Wdocumentation in bitcoin#21631 (#6113) - renaming param while doxygen comment is forgotten in #6114 ## What was done? It contains changes from #6133 Thanks Vijay for spotting issue, this PR is DNM if 6133 will get merged faster. ## How Has This Been Tested? See CI ## 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 bebf391 PastaPastaPasta: utACK bebf391 Tree-SHA512: 34265f09098c04593a9d52709e5b8aed8460248762655945e5c86a65adb9aa49ab6f0bdb21cd907d353fe6b10e1ff2e07d05528166bf8e2140eb2c9ea1acbd33
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was done?
Backports from bitcoin v22:
How Has This Been Tested?
Run unit and functional tests
Breaking Changes
N/A
Checklist: