[27.x] Even more backports #30558

pull fanquake wants to merge 4 commits into bitcoin:27.x from fanquake:27_even_more_backports changing 5 files +15 −6
  1. fanquake commented at 11:06 am on July 31, 2024: member

    Backports:

  2. test: fix constructor of msg_tx 
    In python, if the default value is a mutable object (here: a class)
its shared over all instances, so that one instance being changed
would affect others to be changed as well.
This was likely the source of various intermittent bugs in the
functional tests.

Github-Pull: #30552
Rebased-From: ec5e294e4b830766dcc4a80add0613d3705c1794
    500bba0561
  3. fanquake added this to the milestone 27.2 on Jul 31, 2024
  4. DrahtBot commented at 11:06 am on July 31, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK stickies-v
    Concept ACK petertodd

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  5. DrahtBot added the label Backport on Jul 31, 2024
  6. fanquake marked this as a draft on Jul 31, 2024
  7. in doc/release-notes.md:87 in 1da9dba708 outdated 
    81@@ -74,6 +82,7 @@ Thanks to everyone who directly contributed to this release:
82 - Cory Fields
83 - Martin Zumsande
84 - Max Edwards
85+- Peter Todd
    petertodd commented at 8:36 pm on August 6, 2024:
    Worth crediting @1440000bytes too here, as they finalized my pull-req.
    fanquake commented at 1:14 pm on August 12, 2024:
    Added.
  8. Theschorpioen approved
  9. ariard commented at 9:51 pm on August 6, 2024: contributor

    I would recommend to re-add in the backport release notes that excerpt from 24.x: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-24.0.1.md

    0Contributors to this project strongly recommend that merchants and services
1not accept unconfirmed transactions as final, and if they insist on doing so,
2to take the appropriate steps to ensure they have some recourse or plan for
3when their assumptions do not hold.
  10. petertodd commented at 10:48 am on August 7, 2024: contributor

    @ariard Honestly I’m inclined to think that warning isn’t necessary anymore. 24.x was two years ago, and anyone crazy enough to rely on first-seen has had plenty of warning; I’m not actually aware of any merchants or services still accepting unconfirmed transactions.

    The last one I was aware of was the Chivo Bitcoin ATMs in El Salvador… but they would accept unconfirmed transactions even with BIP-125 opt-in enabled, so that example isn’t actually relevant to this change.

    If anything, I think that wording would actually confuse the issue as you could read it as though there was still a controversy about how safe unconfirmed transactions were. If you were to put in a warning, it should say something like “Bitcoin Core reminds people that unconfirmed transactions can always be cancelled by the sender. Only after at least one confirmation can a transaction be relied upon.”

  11. ariard commented at 6:32 pm on August 7, 2024: contributor

    @petertodd, Yeah it might be too much zeal, yet I’ve been surprised when the option was introduced with 24.x how much services were still claiming to rely zero-conf acceptance. Same, I’m not aware of any bitcoin industry traffic with real-traffic actually accepting unconfirmed transactions (apart of few LSPs with zero-conf, it’s a paradigm of its own and people running that are more savvy about unconf txn risks due to channel funding flow).

    The proposed alternative wording “Bitcoin Core reminds people that unconfirmed transactions can always be cancelled by the sender. Only after at least one confirmation can a transaction be relied upon” sounds indeed better.

  12. fanquake force-pushed on Aug 12, 2024
  13. fanquake marked this as ready for review on Aug 21, 2024
  14. fanquake requested review from willcl-ark on Aug 21, 2024
  15. fanquake requested review from stickies-v on Aug 21, 2024
  16. stickies-v commented at 2:02 pm on August 22, 2024: contributor

    code review ACK https://github.com/bitcoin/bitcoin/commit/885c4b7bc65c294b4b2fac91392d23a37a1f6485 but 8932090f9ecd933c86ee7800c0858e8d1c0bd40b is missing backport metadata in the commit message.

    I verified that all backport commits are clean, except:

    https://github.com/bitcoin/bitcoin/commit/8932090f9ecd933c86ee7800c0858e8d1c0bd40b backported from https://github.com/bitcoin/bitcoin/commit/590456e3f1043ba0680e0afec9fd7653db1098bb: merge conflicts from 539404fe0f and 8950053636 which have been handled in a minimal and straightforward way.

    I think backporting full-rbf makes sense conceptually, yet at the same time I’m a bit worried that shipping a new default value might be considered controversial for maintenance releases where I think people expect to be able to upgrade “automatically” without (non-bug) behaviour change? Although, of course, this exception already exists for consensus changes. Just putting it out there, no strong view.

  17. petertodd commented at 8:42 am on August 23, 2024: contributor

    The cases where enabling full-rbf causes problems are very rare: you need to be running a service that is not vulnerable to unconfirmed double-spends in general. Yet does have some kind of bug related to double spends. And those rare services can still easily turn full-rbf off with a single config setting.

    Meanwhile, the harm to compact block propagation caused by not enabling full-rbf can be seen as a bug that should be fixed.

  18. petertodd commented at 11:12 am on August 23, 2024: contributor

    Yeah it might be too much zeal, yet I’ve been surprised when the option was introduced with 24.x how much services were still claiming to rely zero-conf acceptance.

    Only one service, GAP600, claimed to still rely on zero-conf and they refused to actually provide any examples of clients actually using them. I think it’s highly likely that GAP600 was actually a scam that took peoples’ money and provided nothing of value. Us continuing to claim that zeroconf services still exist is actually helping perpetrate scams like GAP600.

    Same, I’m not aware of any bitcoin industry traffic with real-traffic actually accepting unconfirmed transactions (apart of few LSPs with zero-conf, it’s a paradigm of its own and people running that are more savvy about unconf txn risks due to channel funding flow).

    Zeroconf LSPs are a very different thing. The trust model is explicitly trusting the LSP not to double spend you while the channel is unconfirmed. Phoenix, as an example, uses the BIP125 opt-in bit on their zeroconf channel transactions, so full-RBF settings are irrelevant.

  19. add missing #include <cstdint> for GCC 15 
    Github-Pull: #30633
Rebased-From: 138f8671569f7ebb8c84e9d80c44cddeda9e3845
    ccff378a28
  20. policy/feerate.h: avoid constraint self-dependency 
    In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/format:48,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/bits/chrono_io.h:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/chrono:3362,
                 from ./util/time.h:9,
                 from ./primitives/block.h:12,
                 from ./blockencodings.h:8,
                 from blockencodings.cpp:5:
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits: In substitution of 'template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]':
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1140:25:   required by substitution of 'template<class _Tp, class ... _Args> using std::__is_constructible_impl = std::__bool_constant<__is_constructible(_Tp, _Args ...)> [with _Tp = CFeeRate; _Args = {std::optional<CFeeRate>&}]'
 1140 |       = __bool_constant<__is_constructible(_Tp, _Args...)>;
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:1145:12:   required from 'struct std::is_constructible<CFeeRate, std::optional<CFeeRate>&>'
 1145 |     struct is_constructible
      |            ^~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:178:35:   required by substitution of 'template<class ... _Bn> std::__detail::__first_t<std::integral_constant<bool, false>, typename std::enable_if<(!(bool)(_Bn::value)), void>::type ...> std::__detail::__or_fn(int) [with _Bn = {std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate>}]'
  178 |                                      __enable_if_t<!bool(_Bn::value)>...>;
      |                                                               ^~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/type_traits:196:41:   required from 'struct std::__or_<std::is_constructible<CFeeRate, std::optional<CFeeRate>&>, std::is_convertible<std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, std::optional<CFeeRate> >, std::is_convertible<std::optional<CFeeRate>, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate>&>, std::is_convertible<const std::optional<CFeeRate>&, CFeeRate>, std::is_constructible<CFeeRate, const std::optional<CFeeRate> >, std::is_convertible<const std::optional<CFeeRate>, CFeeRate> >'
  196 |     : decltype(__detail::__or_fn<_Bn...>(0))
      |                ~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:824:45:   required from 'constexpr const bool std::optional<CFeeRate>::__construct_from_contained_value<CFeeRate, CFeeRate>'
  824 |           = !__converts_from_optional<_Tp, _From>::value;
      |                                                    ^~~~~
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:7:   required by substitution of 'template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<CFeeRate>::optional(const std::optional<_Tp>&) [with _Up = CFeeRate]'
  884 |           && __construct_from_contained_value<_Up>
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./validation.h:164:41:   required from here
  164 |         return MempoolAcceptResult(state);
      |                                         ^
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:886:2:   required by the constraints of 'template<class _Tp> template<class _Up>  requires !(is_same_v<std::optional<_Tp>, typename std::remove_cvref<_It2>::type>) && (is_constructible_v<_Tp, const _Up&>) && (__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type>) constexpr std::optional<_Tp>::optional(const std::optional<_From>&)'
/usr/lib/gcc/x86_64-pc-linux-gnu/15/include/g++-v15/optional:884:14: error: satisfaction of atomic constraint '__construct_from_contained_value<_Up, typename std::remove_cv< <template-parameter-1-1> >::type> [with _Tp = _Tp; _Up = _Up]' depends on itself
  884 |           && __construct_from_contained_value<_Up>
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Github-Pull: #30633
Rebased-From: 055bc05792ff5d5b084563044818ebec12bfd748
    57de0f5e77
  21. [WIP] doc: update release notes for 27.x b06c4c6550
  22. fanquake force-pushed on Aug 23, 2024
  23. fanquake commented at 11:18 am on August 23, 2024: member
    I’m just going to backport the other two fixes here, and will follow up with any other changes before the next 27.x.
  24. petertodd commented at 11:19 am on August 23, 2024: contributor
    @fanquake That’s perfectly reasonable. Concept ACK.
  25. stickies-v approved
  26. stickies-v commented at 12:04 pm on August 23, 2024: contributor
    ACK b06c4c6550545351610fc3278dffdd63d5954cf8
  27. DrahtBot requested review from petertodd on Aug 23, 2024
  28. DrahtBot added the label CI failed on Aug 23, 2024
  29. fanquake merged this on Aug 23, 2024
  30. fanquake closed this on Aug 23, 2024
  31. fanquake deleted the branch on Aug 23, 2024


fanquake DrahtBot petertodd Theschorpioen ariard stickies-v


petertodd willcl-ark

Labels
Backport CI failed

Milestone
27.2

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me