backport: bitcoin#16766, #17164, #17283, #17290, #17388, #17437, #17439, #17470, #17568, #17587, #17599 #5314
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
force-pushed
the
bc-bp-v20-missing-3
branch
from
2f9b02f
to
14fe096
Compare
PastaPastaPasta
requested changes
Apr 15, 2023
src/llmq/instantsend.cpp
Outdated
| @@ -1526,7 +1526,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid, const CConnma | |||
| if (nodesToAskFor.size() >= 4) { | |||
| return; | |||
| } | |||
| if (!pnode->m_block_relay_only_peer) { | |||
There was a problem hiding this comment.
Upstream PR doesn't change any invocation of m_block_relay_only_peer why'd you do so here in 17164?
There was a problem hiding this comment.
Because:
// in bitcoin: m_tx_relay == nullptr if we're not relaying transactions with this peer
// in dash: m_tx_relay should never be nullptr, use m_block_relay_only_peer == true instead
std::unique_ptr<TxRelay> m_tx_relay;
Due to #4888
There was a problem hiding this comment.
can we pull this backport into it's own PR? It feel more like a commit inspired by bitcoin's PR rather than just backporting it. And feels more likely to potentially cause issues
| @@ -1026,7 +1027,7 @@ BOOST_FIXTURE_TEST_CASE(util_ChainMerge, ChainMergeTestingSetup) | |||
| // Results file is formatted like: | |||
| // | |||
| // <input> || <output> | |||
| BOOST_CHECK_EQUAL(out_sha_hex, "3e70723862e346ed6e9b48d8efa13d4d56334c0b73fbf3c3a6ac8b8f4d914f65"); | |||
| BOOST_CHECK_EQUAL(out_sha_hex, "f8cff01ad967dfc82cf71208aa2b60f01a0aa621c28356af5ca91d1d7370c217"); | |||
There was a problem hiding this comment.
17388 how'd you calculate this?
There was a problem hiding this comment.
Changed text of message and checked mismatched hash result after running tests
knst
force-pushed
the
bc-bp-v20-missing-3
branch
from
14fe096
to
286476b
Compare
UdjinM6
requested changes
Apr 15, 2023
knst
force-pushed
the
bc-bp-v20-missing-3
branch
from
286476b
to
072a7f6
Compare
knst
force-pushed
the
bc-bp-v20-missing-3
branch
from
072a7f6
to
3acaf35
Compare
|
Looks good but... |
May just need to rebase on develop? |
4671fc3 Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin) 91f3073 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin) 8f174ef Systematize style of IsTrusted single line if (Jeremy Rubin) b49dcbe update variable naming conventions for IsTrusted (Jeremy Rubin) 5ffe0d1 Update comment in test/functional/wallet_balance.py (Jeremy Rubin) a550c58 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin) 5dd7da4 Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin) 595f09d Cache tx Trust per-call to avoid DoS (Jeremy Rubin) dce032c Make IsTrusted scan parents recursively (Jeremy Rubin) Pull request description: This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either. This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected. This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug. The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change. # Before `test_balance()`, we have had two nodes with a balance of 50 # each and then we: # # 1) Sent 40 from node A to node B with fee 0.01 # 2) Sent 60 from node B to node A with fee 0.01 # # Then we check the balances: # # 1) As is # 2) With transaction 2 from above with 2x the fee # # Prior to bitcoin#16766, in this situation, the node would immediately report # a balance of 30 on node B as unconfirmed and trusted. # # After bitcoin#16766, we show that balance as unconfirmed. # # The balance is indeed "trusted" and "confirmed" insofar as removing # the mempool transactions would return at least that much money. But # the algorithm after bitcoin#16766 marks it as unconfirmed because the 'taint' # tracking of transaction trust for summing balances doesn't consider # which inputs belong to a user. In this case, the change output in # question could be "destroyed" by replace the 1st transaction above. # # The post bitcoin#16766 behavior is correct; we shouldn't be treating those # funds as confirmed. If you want to rely on that specific UTXO existing # which has given you that balance, you cannot, as a third party # spending the other input would destroy that unconfirmed. # # For example, if the test transactions were: # # 1) Sent 40 from node A to node B with fee 0.01 # 2) Sent 10 from node B to node A with fee 0.01 # # Then our node would report a confirmed balance of 40 + 50 - 10 = 80 # BTC, which is more than would be available if transaction 1 were # replaced. The release notes have been updated to note the new behavior. ACKs for top commit: ariard: Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR. fjahr: Code review ACK 4671fc3 ryanofsky: Code review ACK 4671fc3. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good! promag: Code review ACK 4671fc3. Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
3645e4c Add missing newline in util_ChainMerge test (Russell Yanofsky) Pull request description: This was causing a lot of test cases not not be very meaningful because multiple configuration options were combined into one line. The changes in test output with this fix make sense and look like: ```diff - testnet=1 regtest=1 || test + testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one. ``` Issue was reported and debugged by Wladimir J. van der Laan <laanwj@protonmail.com> in bitcoin#17385 (comment) <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. --> <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. --> <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. --> ACKs for top commit: laanwj: ACK 3645e4c practicalswift: ACK 3645e4c -- diff looks correct Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165
a5e7795 rpc: Expose block height of wallet transactions (João Barbosa) Pull request description: Closes bitcoin#17296. ACKs for top commit: practicalswift: ACK a5e7795 -- diff looks correct now (good catch @theStack!) theStack: ACK a5e7795 ryanofsky: Code review ACK a5e7795. Changes since last review getblockhash python test fixes, and removing the last hardcoded height Tree-SHA512: 57dcd0e4e7083f34016bf9cf8ef578fbfde49e882b6cd8623dd1c64716e096e62f6177a4c2ed94f5de304e751fe23fb9d11cf107a86fbf0a3c5f539cd2844916
…stants consistently cb9d830 test: Use proper MAX_SCRIPT_ELEMENT_SIZE (Hennadii Stepanov) 402ee70 refactor: Use proper MAX_SCRIPT_ELEMENT_SIZE const (Hennadii Stepanov) Pull request description: This PR replaces well-known "magic" numbers with proper `MAX_SCRIPT_ELEMENT_SIZE` constants. ACKs for top commit: practicalswift: ACK cb9d830 -- diff looks correct and change appears to be complete instagibbs: utACK bitcoin@cb9d830 Tree-SHA512: 5fa033275d6df7e35962c38bfdf09a7b5cd7ef2ccdd5e30a39ba47d0c21ac779a5559c23f5ef5bfd4293be0fc639e836a308bbedf0e34717e1eead983b389bbd
fabd710 ci: Print free disk space (MarcoFalke) fad9fdb test: Properly deserialize integers in little-endian (MarcoFalke) fa94fc1 ci: Run functional tests on s390x (MarcoFalke) Pull request description: Top commit has no ACKs. Tree-SHA512: 98ba77eb56f283131fdaeb393fda86cc308f1bf9781e1e0e5736b8d616528dc8ff2e494d55ba107c138083025c66a59e382fcfa9962d4349a5fd6cbbc52484c3
… code docs 33f5fc3 test: add rpc getaddressinfo labels test coverage (Jon Atack) 0f3539a test: add listlabels test in wallet_labels.py (Jon Atack) 1388de8 rpc: add getaddressinfo code documentation (Jon Atack) 2ee0cb3 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack) 8d1ed0c rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack) 5a0ed85 rpc: improve getaddressinfo RPCHelpman content (Jon Atack) 70cda34 rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack) Pull request description: This PR is a continuation of the work in bitcoin#12892. Main motivations: - There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address. - `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers. Changes by order of commits: - [x] improve/fix getaddressinfo RPCHelpman layout formatting - [x] improve/fix getaddressinfo RPCHelpman content - [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman - [x] update getaddressinfo RPCExamples addresses to bech32 - [x] add getaddressinfo code docs - [x] add a `listlabels` test assertion in wallet_labels.py - [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests Here are gists of the CLI help output: [`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810) [`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba) It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups bitcoin#17578 and bitcoin#17585 now build on this PR._ ACKs for top commit: fjahr: Re-ACK 33f5fc3 jnewbery: ACK 33f5fc3. Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
…subtract fee from outputs b007efd Allow BnB when subtract fee from outputs (Andrew Chow) db15e71 Use BnB when preset inputs are selected (Andrew Chow) Pull request description: Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases. Kind of an alternative to bitcoin#17246 (implements the subtract fee from outputs part of it) and borrows a test from there too. ACKs for top commit: instagibbs: reACK bitcoin@b007efd Sjors: re-ACK b007efd Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
…btractFeeFromOutputs eadd130 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow) ff330ba Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow) Pull request description: bitcoin#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally. Apparently this particular case doesn't have a test, so I've added one as well. ACKs for top commit: Sjors: ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test. fanquake: ACK eadd130 instagibbs: utACK bitcoin@eadd130 Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e
4a96e45 [gui] send: show watch-only balance in send screen (Sjors Provoost) 2689c8f [test] qt: add send screen balance test (Sjors Provoost) Pull request description: Now that we can create a PSBT from a watch-only wallet (bitcoin#16944), we should also display the watch-only balance on the send screen. Before: <img width="1008" alt="before" src="https://user-images.githubusercontent.com/10217/69533384-030e9180-0f78-11ea-9748-c32c957e822e.png"> After: <img width="1009" alt="Schermafbeelding 2019-11-26 om 11 44 17" src="https://user-images.githubusercontent.com/10217/69622879-19811f80-1042-11ea-8279-091012f39b38.png"> I added a test to check the balance on the send screen, but it only covers regular wallets. A better would add a watch-only only wallet. ACKs for top commit: meshcollider: utACK 4a96e45 jb55: utACK 4a96e45 promag: reACK 4a96e45, rebased and label change since last review. instagibbs: code review and light test ACK bitcoin@4a96e45 Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
… we don't need it b6d2183 Minor refactoring to remove implied m_addr_relay_peer. (User) a552e84 added asserts to check m_addr_known when it's used (User) 090b75c p2p: Avoid allocating memory for addrKnown where we don't need it (User) Pull request description: We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay. Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed. Upd: In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default. However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory. This PR still saves memory for block-relay-only peers immediately after merging. Top commit has no ACKs. Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
UdjinM6
force-pushed
the
bc-bp-v20-missing-3
branch
from
3acaf35
to
c0465b4
Compare
PastaPastaPasta
approved these changes
Apr 18, 2023
5 tasks
PastaPastaPasta
pushed a commit
that referenced
this pull request
Apr 19, 2023
…stead (#5339) ## Issue being fixed or feature implemented This refactoring is a follow-up changes to backport bitcoin#17164 (PR #5314) These changes are reduce difference in implementation for our code and bitcoin's ## What was done? Removed a flag m_block_relay_peer. Instead I call IsAddrRelayPeer() that has same information now. It changes logic introduced in #4888 due to dash-specific code. ## How Has This Been Tested? Run unit/functional tests. ## Breaking Changes No breaking changes ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone
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.
Issue being fixed or feature implemented
It's next batch of backports from v20
What was done?
How Has This Been Tested?
Run unit/functional tests
Breaking Changes
No breaking changes. Despite that there are several changes related to RPC (backward compatible).
Checklist: