backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#361, partial: bitcoin#14123 #6234

knst · 2024-08-27T18:26:58Z

Issue being fixed or feature implemented

Regular backports from bitcoin v22

What was done?

See commits for list of backported changes. It also have some missing changes from bitcoin#14123

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A

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

…bled a3f0cbf test: run mempool_reorg.py even with wallet disabled (Darius Parvin) Pull request description: Run mempool_reorg.py test even when the wallet is disabled, as discussed in bitcoin#20078. As part of this PR I created a new method in `MiniWallet`, `create_self_transfer`, to return a raw tx (without broadcasting it) and its associated utxo. ACKs for top commit: MarcoFalke: cr ACK a3f0cbf Tree-SHA512: 316a38faffadcb87499c1d6eca21e9696cef65362bbffcf621788a9b771bb1fa2971b1c7835cbd34b952d7612ad83afbca824cd8be39ecd6b994e8963027f991

…NodeContext 493fb47 Make SetupServerArgs callable without NodeContext (Russell Yanofsky) Pull request description: `bitcoin-gui` code needs to call `SetupServerArgs` but will not have a `NodeContext` object if it is communicating with an external `bitcoin-node` process, so this just passes `ArgsManager` directly. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR bitcoin#10102. ACKs for top commit: MarcoFalke: review ACK 493fb47 Tree-SHA512: 94cda4350113237976e32f1935e3602d1e6ea90c29c4434db2094be70dddf4b63702c3094385258bdf1c3e5b52c7d23bbc1f0282bdd4965557eedd5aef9a0fd4

d7f3b1a Fix gui segfault caused by bitcoin#22216 (Russell Yanofsky) Pull request description: Reported by Hennadii Stepanov bitcoin#22216 (comment) Fixes bitcoin#22227 ACKs for top commit: hebasto: ACK d7f3b1a, tested on Linux Mint 20.1 (Qt 5.12.8). jarolrod: ACK d7f3b1a Tree-SHA512: d672bfa9f1bcd500a879ec7ed27096086ae93b73ad5da8090f29cc5b6d985c46a76583cc384304d67210f87b6b839c2391f0fcc24fd3588c4a014e540283fdfe

…nsaction() 01eedf3 test: doc: improve doc for chain_transaction() helper (Sebastian Falbesoner) 6e63e36 test: refactor: dedup utility function chain_transaction() (Sebastian Falbesoner) Pull request description: Both tests `mempool_packages.py` and `mempool_package_onemore.py` define a utility function `chain_transaction` with a similar implementation. This PR deduplicates it by moving it into the util package and keeping the more general properties: * pass a list of parent_txids/vouts instead of single values * always mark the BIP125-replaceable flag for txs, created via `createrawtransaction` (this is needed by the `mempool_package_onemore.py` test, but doesn't hurt the other one) This is a low-hanging fruit; as a potential follow-up one could probably also deduplicate the function `chain_transaction` in `rpc_packages.py`, which looks a bit different, as it also takes the parent locking script into account and doesn't send the tx. ACKs for top commit: mjdietzx: reACK 01eedf3 klementtan: Code review ACK 01eedf3 MarcoFalke: review ACK 01eedf3 🙅 Tree-SHA512: ac7105d02c23f53d76d4ec9dc8de1074dd8faefeecd44b107921b78665279498966152fed312ecbe252a1c34a9643d531166329a4fea0e773df3bb75d43092b0

…ing RBF test fa7d71f test: Run pep-8 on touched test (MarcoFalke) fab7e99 test: Use MiniWallet in test_no_inherited_signaling RBF test (MarcoFalke) fab871f test: Remove unused generate() from test (MarcoFalke) faff3f3 test: Add txin.sequence option to MiniWallet (MarcoFalke) Pull request description: This comes with nice benefits: * Less code and complexity * Test can be run without wallet compiled in Also add some additional checks for `getmempoolentry` (bitcoin#22209) and other cleanups 🎨 ACKs for top commit: mjdietzx: Tested ACK fa7d71f thanks for the explanations, nicely done theStack: ACK fa7d71f 🍷 Tree-SHA512: 0e9b8fe985779d8d7034d256deed627125bb374b6ae2972c461b3a220739a51061c6147ad69339bee16282f82716c7f3f8a7a89c693ceb1e47ea50709272332a

…nd check tx vsize d6d2ab9 test: MiniWallet: fix fee calculation for P2PK and check tx vsize (Sebastian Falbesoner) ce024b1 test: MiniWallet: force P2PK signature to have fixed size (71 bytes) (Sebastian Falbesoner) Pull request description: This PR is a follow-up to bitcoin#21945. It aims to both fix the fee calculation for P2PK mode transactions and enable its vsize check. Currently, the latter assumes a fixed tx length, which is fine for anyone-can-spend txs but doesn't apply to P2PK output spends due to varying DER signature size; the vsize check is therefore disabled for P2PK mode on master branch. Creating one million DER signatures with MiniWallet shows the following distribution of sizes (smart people with better math skills probably could deduce the ratios without trying, but hey): | DER signature size [bytes] | #occurences (ratio) | | ------------- | ------------- | | 71 | 498893 (49.89%) | | 70 | 497244 (49.72%) | | 69 | 3837 (0.38%) | | 68 | 22 (0.0022%) | Note that even smaller signatures are possible (for smaller R and S values with leading zero bytes), it's just that the probability decreases exponentially. Instead of choosing a large vsize check range and hoping that smaller signatures are never created (potentially leading to flaky tests), the proposed solution is ~~to limit the signature size to the two most common sizes 71 and 70 (>99.6% probability) and then accordingly only check for two vsize values; the value to be used for fee calculation is a decimal right between the two possible sizes (167.5 vbytes) and for the vsize check it's rounded down/up integer values are used.~~ to simply grind the signature to a fixed size of 71 bytes (49.89% probability, i.e. on average each call to `sign_tx()`, on average two ECC signing operations are needed). ~~The idea of grinding signatures to a fixed size (similar to bitcoin#13666 which grinds to low-R values) would be counter-productive, as the signature creation in the test suite is quite expensive and this would significantly slow down tests that calculate hundreds of signatures (like e.g. feature_csv_activation.py).~~ For more about transaction sizes on different input/output types, see the following interesting article: https://medium.com/coinmonks/on-bitcoin-transaction-sizes-97e31bc9d816 ACKs for top commit: MarcoFalke: Concept ACK d6d2ab9 Tree-SHA512: 011c70ee0e4adf9ba12902e4b6c411db9ae96bdd8bc810bf1d67713367998e28ea328394503371fc1f5087a819547ddaea56c073b28db893ae1c0031d7927f32

knst · 2024-08-29T10:27:46Z

I split non-v22 backports to new PR #6240

PastaPastaPasta · 2024-08-29T19:23:30Z

src/qt/bitcoingui.cpp

-UnitDisplayStatusBarControl::UnitDisplayStatusBarControl() :
-    optionsModel(nullptr),
-    menu(nullptr)
+UnitDisplayStatusBarControl::UnitDisplayStatusBarControl()
+    : optionsModel(nullptr),
+      menu(nullptr)


3803280: these changes feel unrelated

3803280: these changes feel unrelated

they are from original PR: https://github.com/bitcoin-core/gui/pull/275/files#diff-2ecf8cbf369cf3d2f3d2b1cf5cfe4c1a647d63e11e2885d2fd0ac11fb5f7a804R1453-R1456

gui 375 feels maybe incomplete, but I didn't dig into it. one other nit

Most changes in gui-375 is related to platform style but we don't have it from the very beginning.
So, maybe I can park it as a partial?

PastaPastaPasta

generally utACK 3803280

gui 375 feels maybe incomplete, but I didn't dig into it. one other nit

UdjinM6 · 2024-08-31T10:32:28Z

src/qt/coincontroldialog.cpp

+    if (e->type() == QEvent::PaletteChange) {
+        updateView();
+    }


gui 275: this has no effect for us because our gui themes do not care about os palettes/themes. I'd suggest to drop this backport completely and mark it DNM.

@PastaPastaPasta

gui 375 feels maybe incomplete, but I didn't dig into it. one other nit

gui-275 is dropped

UdjinM6

utACK be5c84f

PastaPastaPasta

utACK be5c84f

knst and others added 7 commits August 28, 2024 01:09

fix: missing changes from bitcoin#14123

f1f5723

knst changed the title ~~backport: bitcoin#14123, #21178, #22089, #22130, #22210, #22216, #275, #27920, #28799~~ backport: bitcoin#21178, #22089, #22130, #22210, #22216, #275, #28799, partial: #14123, 27920 Aug 27, 2024

knst force-pushed the bp-v22-p17 branch from cc5d25a to ad17c63 Compare August 28, 2024 10:32

knst changed the title ~~backport: bitcoin#21178, #22089, #22130, #22210, #22216, #275, #28799, partial: #14123, 27920~~ backport: bitcoin#21178, #22089, #22130, #22210, #22216, #275, #28799, partial: #14123, #27920, #29007 Aug 29, 2024

knst added this to the 21.2 milestone Aug 29, 2024

knst requested review from UdjinM6 and PastaPastaPasta August 29, 2024 10:19

knst marked this pull request as ready for review August 29, 2024 10:21

knst changed the title ~~backport: bitcoin#21178, #22089, #22130, #22210, #22216, #275, #28799, partial: #14123, #27920, #29007~~ backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#275, bitcoin-core/gui#361, partial: #14123, #27920, #29007 Aug 29, 2024

knst force-pushed the bp-v22-p17 branch from 48e4cb7 to 3803280 Compare August 29, 2024 10:24

PastaPastaPasta reviewed Aug 29, 2024

View reviewed changes

PastaPastaPasta previously approved these changes Aug 29, 2024

View reviewed changes

UdjinM6 reviewed Aug 31, 2024

View reviewed changes

knst dismissed PastaPastaPasta’s stale review via be5c84f September 3, 2024 10:59

knst force-pushed the bp-v22-p17 branch from 3803280 to be5c84f Compare September 3, 2024 10:59

knst changed the title ~~backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#275, bitcoin-core/gui#361, partial: bitcoin#14123~~ backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#361, partial: bitcoin#14123 Sep 3, 2024

knst requested review from PastaPastaPasta and UdjinM6 September 3, 2024 11:00

UdjinM6 approved these changes Sep 3, 2024

View reviewed changes

PastaPastaPasta approved these changes Sep 3, 2024

View reviewed changes

PastaPastaPasta merged commit a83f76b into dashpay:develop Sep 3, 2024
33 of 38 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#361, partial: bitcoin#14123 #6234

backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#361, partial: bitcoin#14123 #6234

knst commented Aug 27, 2024 •

edited

Loading

knst commented Aug 29, 2024

PastaPastaPasta Aug 29, 2024

knst Aug 30, 2024

PastaPastaPasta left a comment

UdjinM6 Aug 31, 2024

knst Sep 3, 2024

UdjinM6 left a comment

PastaPastaPasta left a comment

backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#361, partial: bitcoin#14123 #6234

backport: bitcoin#21178, #22089, #22130, #22210, #22216, bitcoin-core/gui#361, partial: bitcoin#14123 #6234

Conversation

knst commented Aug 27, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

knst commented Aug 29, 2024

PastaPastaPasta Aug 29, 2024

Choose a reason for hiding this comment

knst Aug 30, 2024

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 Aug 31, 2024

Choose a reason for hiding this comment

knst Sep 3, 2024

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst commented Aug 27, 2024 •

edited

Loading