Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[27.x] Even more backports #30558

Merged
merged 4 commits into from
Aug 23, 2024
Merged

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Jul 31, 2024

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: bitcoin#30552
Rebased-From: ec5e294
@fanquake fanquake added this to the 27.2 milestone Jul 31, 2024
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 31, 2024

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.

@fanquake fanquake marked this pull request as draft July 31, 2024 16:21
doc/release-notes.md Outdated Show resolved Hide resolved
@ariard
Copy link
Contributor

ariard commented Aug 6, 2024

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

Contributors to this project strongly recommend that merchants and services
not accept unconfirmed transactions as final, and if they insist on doing so,
to take the appropriate steps to ensure they have some recourse or plan for
when their assumptions do not hold.

@petertodd
Copy link
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."

@ariard
Copy link
Contributor

ariard commented Aug 7, 2024

@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.

@fanquake fanquake force-pushed the 27_even_more_backports branch from 1da9dba to 8f411b3 Compare August 12, 2024 13:13
@fanquake fanquake marked this pull request as ready for review August 21, 2024 20:01
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code review ACK 885c4b7 but 8932090 is missing backport metadata in the commit message.

I verified that all backport commits are clean, except:

8932090 backported from 590456e: merge conflicts from 539404f and 8950053 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.

@petertodd
Copy link
Contributor

petertodd commented Aug 23, 2024 via email

@petertodd
Copy link
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.

whitslack and others added 3 commits August 23, 2024 12:16
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: bitcoin#30633
Rebased-From: 055bc05
@fanquake fanquake force-pushed the 27_even_more_backports branch from 8f411b3 to b06c4c6 Compare August 23, 2024 11:17
@fanquake
Copy link
Member Author

I'm just going to backport the other two fixes here, and will follow up with any other changes before the next 27.x.

@petertodd
Copy link
Contributor

@fanquake That's perfectly reasonable. Concept ACK.

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b06c4c6

@DrahtBot DrahtBot requested a review from petertodd August 23, 2024 12:04
@fanquake fanquake merged commit 84df309 into bitcoin:27.x Aug 23, 2024
15 of 16 checks passed
@fanquake fanquake deleted the 27_even_more_backports branch August 23, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants