backport: bitcoin#23590, 23061, 23694 #6256

vijaydasmp · 2024-09-07T04:13:38Z

This backport contains following updates

Crash on Mempool ConsensusScriptChecks Failure: Added a fix to crash debug builds when ConsensusScriptChecks in the mempool fails. This prevents bugs from bypassing the testing infrastructure by forcing a crash during tests.
Fix for -persistmempool Option: Corrected the behavior of -persistmempool, which was previously interpreted as -nopersistmempool. This update ensures the flag works as intended.
Documentation: Added the missing "optional" field to the MempoolEntryDescription documentation for better clarity.

vijaydasmp · 2024-09-09T15:01:41Z

Hello @UdjinM6 requesting review

UdjinM6

utACK 376cb95

vijaydasmp · 2024-09-12T00:28:55Z

Hello @PastaPastaPasta, @knst , requesting review

knst · 2024-09-24T07:53:21Z

src/wallet/wallet.cpp

    AssertLockNotHeld(cs_wallet);
    // Skip the queue-draining stuff if we know we're caught up with
-    // chainActive.Tip(), otherwise put a callback in the validation interface queue and wait
+    // chain.Tip(), otherwise put a callback in the validation interface queue and wait


23258: nit: missing parenthesis

chain -> chain()

knst · 2024-09-24T07:58:30Z

doc/release-notes-23061.md

+- In previous releases, the meaning of the command line option
+  `-persistmempool` (without a value provided) incorrectly disabled mempool
+  persistence.  `-persistmempool` is now treated like other boolean options to
+  mean `-persistmempool=1`. Passing `-persistmempool=0`, `-persistmempool=1`
+  and `-nopersistmempool` is unaffected. (#)


23061: we don't need a release note for it, because this issue has been fixed very long time ago by kwvg here 18514d3 (#5546)

Please, keep only changes in test/functional/mempool_persist.py for this backport

vijaydasmp · 2024-09-27T03:23:30Z

Hello @knst , requesting review

UdjinM6

utACK 65821b9

knst

utACK 65821b9

knst

I got locally a crash (assert) on this code, I am checking more.

Logs:

validation.cpp:810 ConsensusScriptChecks: Assertion `false' failed.
Posix Signal: Aborted
   0#: (0x5E97E5A39494) stl_vector.h:115        - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_copy_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data const&)
   1#: (0x5E97E5A39494) stl_vector.h:127        - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_swap_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data&)
   2#: (0x5E97E5A39494) stl_vector.h:1959       - std::vector<unsigned long, std::allocator<unsigned long> >::_M_move_assign(std::vector<unsigned long, std::allocator<unsigned long> >&&, std::integral_constant<bool, true>)
   3#: (0x5E97E5A39494) stl_vector.h:768        - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&)
   4#: (0x5E97E5A39494) stacktraces.cpp:777     - HandlePosixSignal
   5#: (0x7F1353442990) libc_sigaction.c        - ???
   6#: (0x7F1353499A1B) pthread_kill.c:44       - __pthread_kill_implementation
   7#: (0x7F1353499A1B) pthread_kill.c:78       - __pthread_kill_internal
   8#: (0x7F1353499A1B) pthread_kill.c:89       - __GI___pthread_kill
   9#: (0x7F13534428E6) raise.c:27              - __GI_raise
  10#: (0x7F13534268B7) abort.c:81              - __GI_abort
  11#: (0x5E97E519BD58) basic_string.h:792      - std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::~basic_string()
  12#: (0x5E97E519BD58) check.cpp:14            - assertion_fail(char const*, int, char const*, char const*)
  13#: (0x5E97E55CB53A) validation.h:118        - TxValidationState::TxValidationState(TxValidationState const&)
  14#: (0x5E97E55CB53A) validation.cpp:883      - AcceptSingleTransaction
  15#: (0x5E97E55CB53A) validation.cpp:977      - AcceptToMemoryPoolWithTime
  16#: (0x5E97E55CC465) validation.cpp:999      - AcceptToMemoryPool(CChainState&, CTxMemPool&, std::shared_ptr<CTransaction const> const&, bool, bool)
  17#: (0x5E97E53BD299) transaction.cpp:70      - BroadcastTransaction(NodeContext&, std::shared_ptr<CTransaction const>, bilingual_str&, long const&, bool, bool, bool)
  18#: (0x5E97E5519F32) shared_ptr_base.h:1070  - std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count()
  19#: (0x5E97E5519F32) shared_ptr_base.h:1524  - std::__shared_ptr<CTransaction const, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr()
  20#: (0x5E97E5519F32) shared_ptr.h:175        - std::shared_ptr<CTransaction const>::~shared_ptr()
  21#: (0x5E97E5519F32) rawtransaction.cpp:1162 - operator()
  22#: (0x5E97E551A1E4) std_function.h:292      - _M_invoke
  23#: (0x5E97E59CE276) util.cpp:521            - RPCHelpMan::HandleRequest(JSONRPCRequest const&) const
  24#: (0x5E97E544C16A) univalue.h:17           - UniValue::operator=(UniValue&&)
  25#: (0x5E97E544C16A) server.h:108            - CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const
  26#: (0x5E97E554B9D4) std_function.h:591      - std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const
  27#: (0x5E97E554B9D4) server.cpp:622          - ExecuteCommand
  28#: (0x5E97E554CA7F) server.cpp:511          - ExecuteCommands
  29#: (0x5E97E554CA7F) server.cpp:543          - CRPCTable::execute(JSONRPCRequest const&) const

vijaydasmp · 2024-10-01T13:16:11Z

Hi @knst should I rebase it with develop?

knst · 2024-10-01T13:22:54Z

Hi @knst should I rebase it with develop?

yes, please rebase it with develop just to let everyone see this failure in CI without running own local copy to reproduce issue

I am investigating issue already, I will update about status later or maybe provide a fix

UdjinM6 · 2024-10-01T14:28:12Z

@knst pls check #6298

c91ba8a fix: no crashes allowed (UdjinM6) a4cd1d6 fix: explicitly test no tx in mempool after invalidateblock (UdjinM6) 04b5db9 test: extend and refactor DIP0020 activation test (UdjinM6) Pull request description: ## Issue being fixed or feature implemented ~23590 backport in #6256 results in node crashes (for dashd compiled with `--enable-debug`) when we try to spend coins locked via disabled opcodes in `feature_dip0020_activation.py`. Also,~ there is only rpc part and no tests for relaying such txes via p2p. ## What was done? ## How Has This Been Tested? run test on develop and on top of #6256 + #6299 with and without `--enable-debug` ## Breaking Changes ## Checklist: - [ ] 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 - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK c91ba8a knst: utACK c91ba8a Tree-SHA512: 2ba16d6a6bb58cb98c01234ed60a8eecd4ff214d3d8386a4b8ed10f4776e0862d7794747791d82345d6031678a308df39c2dbdd361a902ee1e56cf7f05a73c1a

knst · 2024-10-04T09:09:07Z

I convert this one in Draft until one of these PRs are not merged: #6302 or #6299 @vijaydasmp

github-actions · 2024-10-07T20:01:30Z

This pull request has conflicts, please rebase.

…ivated long time ago c8fd37d docs: added a comment about removed SCRIPT_ENABLE_DIP0020_OPCODES (Konstantin Akimov) 61bc300 feat: drop SCRIPT_ENABLE_DIP0020_OPCODES, make opcodes available from genesis block (Konstantin Akimov) 0e55abd feat: remove feature_dip0020_activationl.py functional test and related code (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented it's alternate solution for #6299 to fix a crash reported #6256 ## What was done? Removed code related to DIP0020 activation for various OP codes. DIP0020 is activated long time ago and no any historical blocks are violating rules, removing it's backwards compatible. ## How Has This Been Tested? Run unit and functional tests. See also changes in data for unit tests and removed functional test. It also re-index mainnet and testnet successfully ``` src/qt/dash-qt -reindex -assumevalid=0 src/qt/dash-qt -testnet -reindex -assumevalid=0 ``` Also extra test is done with bitcoin#23590 - no crash with it in `feature_dip0020_activation.py` [modified assuming it is always activated] ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK c8fd37d PastaPastaPasta: utACK c8fd37d Tree-SHA512: 05ddda4e8fb66305995e91c8a04fbda690aef8fb82acb23b7d62f302da60b5ec7e7a97bd988efd2523dbd9cafde9f4b65cae2db9e4b5257464ce1c8fcca6a40f

knst · 2024-10-15T14:20:02Z

@vijaydasmp #6302 is merged, please, rebase this PR to the top of develop

…hecks fails faad05c Crash debug builds when mempool ConsensusScriptChecks fails (MarcoFalke) Pull request description: Currently a bug in the function might sneak around our testing infrastructure. Fix that by turning bugs into crashes during tests. ACKs for top commit: glozow: utACK faad05c, there's something seriously wrong with the code if this returns false, good to throw in debug mode Tree-SHA512: dfea1cd9ce3f1c303f49cca1417cd5c77c6ed12849aaff7b6ab1b6060f2f0c9cf5d4689017355d11f66639bab35823f65f848e6979042fa875181509dfd5d3d7

faa9c19 doc: Add 23061 release notes (MarcoFalke) faff17b Fix (inverse) meaning of -persistmempool (MarcoFalke) Pull request description: Passing `-persistmempool` is currently treated as `-nopersistmempool` ACKs for top commit: jnewbery: reACK faa9c19 hebasto: ACK faa9c19, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: f34a89a07745dabe340eb845b2a348b79c093e9056f7a21c17e1ba2e278177c9b4cf30e8095791fd645a7f90eb34850b2eee0c869b4f6ec02bf749c73b0e52ee

…tion fa1571b doc: Add missing optional to MempoolEntryDescription (MarcoFalke) Pull request description: Needed for bitcoin#23083. Can be reviewed with `--word-diff-regex=.`. ACKs for top commit: josibake: ACK bitcoin@fa1571b shaavan: ACK fa1571b Tree-SHA512: b4370003d2aeadce438778e15bd9a0d6a7fef4711acbe8471a50a9d72bbf74e1705fecbaae6f7eb367ece7c795a816c4b8b6583ed6c8f91b35621ca30fd95c18

UdjinM6

utACK 7d170bd

vijaydasmp · 2024-10-21T07:39:07Z

Hello @knst @PastaPastaPasta , please review

PastaPastaPasta

utACK 7d170bd

should be fixed now

, bitcoin#23804, bitcoin#24310, bitcoin#24537, bitcoin#24582, bitcoin#24404, bitcoin#25013, bitcoin#25029, bitcoin#25254, bitcoin#25388 (mempool backports) 578be36 merge bitcoin#25388: move policy constants to policy (Kittywhiskers Van Gogh) aca2c0d merge bitcoin#25254: Move minRelayTxFee to policy/settings (Kittywhiskers Van Gogh) 1d82dd8 merge bitcoin#25029: Move fee estimation RPCs to separate file (Kittywhiskers Van Gogh) ad94a30 merge bitcoin#25013: Remove cs_main from verifymessage, move msg utils to new file (Kittywhiskers Van Gogh) 8e42b12 merge bitcoin#24404: Remove confusing P1008R1 violation in ATMPArgs (Kittywhiskers Van Gogh) 006370d merge bitcoin#24582: Move txoutproof RPCs to txoutproof.cpp (Kittywhiskers Van Gogh) dfa4772 merge bitcoin#24537: Split mempool RPCs from blockchain.cpp (Kittywhiskers Van Gogh) 537d3b3 merge bitcoin#24310: docs / fixups from RBF and packages (Kittywhiskers Van Gogh) 8239d5f merge bitcoin#23804: followups for de-duplication of packages (Kittywhiskers Van Gogh) 71b2623 merge bitcoin#22674: mempool validation and submission for packages of 1 child + parents (Kittywhiskers Van Gogh) 8392d23 merge bitcoin#23381: refactoring for package submission (Kittywhiskers Van Gogh) d3c0059 merge bitcoin#22689: deprecate top-level fee fields in getmempool RPCs (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * [bitcoin#23694](bitcoin#23694) (backported in [dash#6256](#6256)) was backported before [bitcoin#22689](bitcoin#22689) and therefore, the formatting changes in the former were not incorporated in the backport. This has been resolved by accommodating them in the latter. * The fields deprecated in [bitcoin#22689](bitcoin#22689) were originally deprecated in [bitcoin#12240](bitcoin#12240) but not gated behind runtime arguments, the backport does so. * This pull request includes multiple backports that include code specific to RBF (BIP 125) and SegWit, which have been removed due to Dash Core not supporting them, comments and documentation (like `doc/policy/packages.md`) have been amended to reflect this accordingly. * When backporting [bitcoin#22674](bitcoin#22674), a new constructor was introduced for submitting `MEMPOOL_ENTRY` `ResultType`, upstream this can be done trivially as it uses function overloading ([source](https://github.com/bitcoin/bitcoin/pull/22674/files#diff-d3c243938494b10666b44404a27af7d84b44a72b85a27431e0c89e181462ca6eR198-R204)) but as Dash Core doesn't accept `replaced_txns`, there is no opportunity to differentiate arguments. * This was resolved by accepting `ResultType` as an argument but always passing `MEMPOOL_ENTRY` as the argument ([source](71b2623#diff-d3c243938494b10666b44404a27af7d84b44a72b85a27431e0c89e181462ca6eR232-R242)) * Unit tests like `package_witness_swap_tests` (introduced in [bitcoin#23804](bitcoin#23804)) have been omitted due to functionality tested not being implemented in Dash Core. Subsequent backports that include modifications to these tests have mirrored similar omissions. ## Breaking Changes * The top-level fee fields `fee`, `modifiedfee`, `ancestorfees` and `descendantfees` returned by RPCs `getmempoolentry`,`getrawmempool(verbose=true)`, `getmempoolancestors(verbose=true)` and `getmempooldescendants(verbose=true)` are deprecated and will be removed in the next major version (use `-deprecated=fees` if needed in this version). The same fee fields can be accessed through the `fees` object in the result. **WARNING: deprecated fields `ancestorfees` and `descendantfees` are denominated in sats, whereas all fields in the `fees` object are denominated in DASH.** ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 578be36 PastaPastaPasta: utACK 578be36 Tree-SHA512: 38a62ec826f91c349269373463dd47f662cdc14dbb3c82cb5c56e779f22a2623fdce055018f15bcf6b5da029312dbcce0925545650c0eae8e3f5177812b68321

vijaydasmp changed the title ~~backport: bitcoin#~~ backport: bitcoin#23590, 23528, 2306 Sep 7, 2024

vijaydasmp changed the title ~~backport: bitcoin#23590, 23528, 2306~~ backport: bitcoin#23590, 23528, 23061 Sep 7, 2024

vijaydasmp changed the title ~~backport: bitcoin#23590, 23528, 23061~~ backport: bitcoin#23590, 23528, 23061, 23694 Sep 8, 2024

vijaydasmp changed the title ~~backport: bitcoin#23590, 23528, 23061, 23694~~ backport: bitcoin#23590, 23258, 23061, 23694 Sep 8, 2024

vijaydasmp marked this pull request as ready for review September 8, 2024 06:25

UdjinM6 previously approved these changes Sep 11, 2024

View reviewed changes

UdjinM6 added this to the 21.2 milestone Sep 22, 2024

UdjinM6 requested review from PastaPastaPasta and knst September 22, 2024 15:43

knst requested changes Sep 24, 2024

View reviewed changes

vijaydasmp dismissed UdjinM6’s stale review via 65821b9 September 24, 2024 14:48

vijaydasmp force-pushed the bp23_2024_09_01 branch from 376cb95 to 65821b9 Compare September 24, 2024 14:48

vijaydasmp requested review from UdjinM6 and knst September 24, 2024 14:49

UdjinM6 previously approved these changes Sep 27, 2024

View reviewed changes

knst approved these changes Sep 27, 2024

View reviewed changes

knst previously requested changes Oct 1, 2024

View reviewed changes

UdjinM6 mentioned this pull request Oct 1, 2024

test: extend and refactor DIP0020 activation test #6298

Merged

5 tasks

UdjinM6 mentioned this pull request Oct 1, 2024

fix: Exclude SCRIPT_ENABLE_DIP0020_OPCODES from flags in mempool checks while DIP0020 is not activated yet #6299

Closed

5 tasks

vijaydasmp force-pushed the bp23_2024_09_01 branch from 65821b9 to 7cb521c Compare October 2, 2024 02:45

vijaydasmp requested a review from knst October 2, 2024 10:31

knst marked this pull request as draft October 4, 2024 09:09

github-actions bot added the needs rebase label Oct 7, 2024

knst mentioned this pull request Oct 10, 2024

feat: remove DIP0020 requirement for OP_ since it is activated long time ago #6302

Merged

5 tasks

MarcoFalke added 3 commits October 17, 2024 20:19

vijaydasmp dismissed UdjinM6’s stale review via 7d170bd October 17, 2024 15:00

vijaydasmp force-pushed the bp23_2024_09_01 branch from 7cb521c to 7d170bd Compare October 17, 2024 15:00

github-actions bot removed the needs rebase label Oct 17, 2024

vijaydasmp changed the title ~~backport: bitcoin#23590, 23258, 23061, 23694~~ backport: bitcoin#23590, 23061, 23694 Oct 17, 2024

vijaydasmp marked this pull request as ready for review October 17, 2024 16:32

vijaydasmp requested a review from UdjinM6 October 18, 2024 04:11

UdjinM6 approved these changes Oct 18, 2024

View reviewed changes

PastaPastaPasta approved these changes Oct 21, 2024

View reviewed changes

PastaPastaPasta merged commit 40dc6d9 into dashpay:develop Oct 21, 2024
36 checks passed

UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024

kwvg mentioned this pull request Dec 24, 2024

backport: merge bitcoin#22689, #23381, #22674, #23804, #24310, #24537, #24582, #24404, #25013, #25029, #25254, #25388 (mempool backports) #6499

Merged

5 tasks

backport: bitcoin#23590, 23061, 23694 #6256

backport: bitcoin#23590, 23061, 23694 #6256

Uh oh!

Conversation

vijaydasmp commented Sep 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vijaydasmp commented Sep 9, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

vijaydasmp commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

knst Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

vijaydasmp commented Sep 27, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

vijaydasmp commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst commented Oct 1, 2024

Uh oh!

UdjinM6 commented Oct 1, 2024

Uh oh!

knst commented Oct 4, 2024

Uh oh!

github-actions bot commented Oct 7, 2024

Uh oh!

knst commented Oct 15, 2024

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

vijaydasmp commented Oct 21, 2024

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vijaydasmp commented Sep 7, 2024 •

edited

Loading

vijaydasmp commented Sep 12, 2024 •

edited

Loading

vijaydasmp commented Oct 1, 2024 •

edited

Loading