CPFP fixups and followups #332
Merged
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
| fn cpfp_package( | ||
| revaultd: &Arc<RwLock<RevaultD>>, | ||
| bitcoind: &BitcoinD, | ||
| mut tx_package: Vec<impl CpfpableTransaction>, | ||
| current_feerate: u64, | ||
| mut tx_package: Vec<ToBeCpfped>, |
There was a problem hiding this comment.
Can we call this a package?
There was a problem hiding this comment.
Not really, i fix it elsewhere IIRC. If not here, the rationale was that i didn't want to increase the diff by too much.
|
Fixed your comment on the tests. |
Don't needlessly spam more than we already do..
|
Rebased on master for Github aesthetics |
There was a problem hiding this comment.
ACK 5ae472c
I'll re-ack if you decide to set the revault_tx reference to master :)
Cargo.toml
Outdated
| @@ -21,8 +21,8 @@ path = "src/bin/cli.rs" | |||
| bench = [] | |||
|
|
|||
| [dependencies] | |||
| revault_tx = { git = "https://github.com/danielabrozzoni/revault_tx/", branch = "2021109_support_cpfp", features = ["use-serde"] } | |||
| revault_net = { git = "https://github.com/danielabrozzoni/revault_net/", branch = "revault_tx_update" } | |||
| revault_tx = { git = "https://github.com/darosior/revault_tx", branch = "cpfp_followupd", features = ["use-serde"] } | |||
There was a problem hiding this comment.
I suppose this can be set to master now?
|
Uhm I just saw #302 (comment), I'll fix in #331 after this is merged |
|
Yeah i need to update revault_net first
Sent from ProtonMail mobile
…-------- Original Message --------
On Dec 14, 2021, 12:05, Daniela Brozzoni wrote:
@danielabrozzoni commented on this pull request.
ACK [5ae472c](5ae472c)
I'll re-ack if you decide to set the revault_tx reference to master :)
---------------------------------------------------------------
In [Cargo.toml](#332 (comment)):
> @@ -21,8 +21,8 @@ path = "src/bin/cli.rs"
bench = []
[dependencies]
-revault_tx = { git = "https://github.com/danielabrozzoni/revault_tx/", branch = "2021109_support_cpfp", features = ["use-serde"] }
-revault_net = { git = "https://github.com/danielabrozzoni/revault_net/", branch = "revault_tx_update" }
+revault_tx = { git = "https://github.com/darosior/revault_tx", branch = "cpfp_followupd", features = ["use-serde"] }
I suppose this can be set to master now?
—
You are receiving this because you authored the thread.
Reply to this email directly, [view it on GitHub](#332 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F72NYYKP2OD53SLI6LUQ4QITANCNFSM5J6CBMGA).
Triage notifications on the go with GitHub Mobile for [iOS](https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or [Android](https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub).
|
A nit, but it makes the check and assumption on the two vectors lengths closer. Also rename the 'listunspent' var for being self-descriptive.
This was incorrectly considering the set of unvaults (or spends) as "a package". It's misleading as a package is usually used to refer to linked transactions in Bitcoin (mempool) land. Maybe this confusion caused the error: it considered the entire set of transactions' feerate as if they somehow had an influence on each other wrt mining. Instead, filter transactions to be bumped individually. In addition we introduce a threshold below which we don't feebump (probably not worth paying for a CPFP if you are like 2sat/vb below the next block feerate!).
Avoid two races, and sanity check the tx-cpfp pairs feerate.
|
Update revault_tx and revault_net to their respective master |
Make it huge so we can generate loads of blocks on CI without any issue
|
Spurious HTTP timeout when generating blocks on CI: added a commit (unrelated to this PR, but hey CI won't be flaky then) making the timeout much larger so we are now fine with generating loads of blocks at once. |
|
ACK 580ba22 |
Merged
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.
This is a followup to #302, fixing inconsistencies and cleaning up a bit.
The revault_tx counterpart is at revault/revault_tx#117 .