Repository navigation
standardize rewards to pool/solo mining into a single scriptPubKey - #1800
Conversation
|
there's broken tests on CI |
|
the channel APIs (e.g.: that's because those APIs were designed for more general usage that is not limited by the the simplified implementations we have on the application layer here more specifically, aside from the pool/solo reward, some opinionated mining server could choose to add merged mining outputs, or some arbitrary OP_RETURN, or even split rewards across multiple outputs without ever relying on our with that said, I'd make |
|
comments above are only after a high-level analysis, I haven't looked deeply at the PR yet |
a4a20d2 to
7443eaa
Compare
7443eaa to
998290e
Compare
|
@plebhash , thanks for the insights! I think this is ready for review now! |
427dc7b to
673ca61
Compare
I guess the PR description is now outdated, which could be a bit misleading to the review process. |
Adjusted |
673ca61 to
a5c51ba
Compare
|
rebased and addressed @shourya742's comment |
|
while doing my final review here, I started feeling that so I went ahead and tried to get rid of them. I was successful for Pool, so I'd encourage the following commits to be cherry-picked here: for JDC that was not possible, mostly due to the fact that for JDS it was possible, however JDS contains some unit tests that depend on tbh, those unit tests are pretty generic and I feel they should belong to @lucasbalieiro can you try to migrate these unit tests from JDS to
obviously, they should also be renamed, since strictly speaking, we don't really a need and then, once those tests are migrated, let's also get rid of |
|
@plebhash, just following up on the points you raised in the previous #1800 (comment). Here’s what I’ve already addressed:
Now, regarding the migration of the tests to The tests in question validate things like empty and non-empty scripts, which seem generic at first, but they’re really checking the parsing from text into the Migrating these tests would also require adding Besides that, placing these tests in With that in mind, I’d suggest keeping the tests where they are, as in this commit: As I said, what you’re asking is totally doable. I just think this approach makes more sense. What you think? |
|
@lucasbalieiro thanks for the deeper analysis I think the approach you took makes sense |
…validate the new single coinbase output
…tern applied for the `CoinbaseRewardScript`
…ern applied for the `CoinbaseRewardScript`
…ern applied for the `CoinbaseRewardScript`
…nbase_reward_script` and adapt the code where needed
…eward_script` and adapt the code where needed
…eward_script` and adapt the code where needed
…coinbase_reward_script\'
b70fb49 to
7dfc5b4
Compare
closes #1793
This refactor modifies the handling of coinbase outputs across multiple roles by replacing the
CoinbaseOutputtype withCoinbaseRewardScript. The change simplifies the configuration and enforces a single coinbase output script, eliminating the need for a vector of outputs.Additionally, the
deserialize_vec_exactly_1function is removed since it is no longer necessary.Adjusted the usage of
Vec<TxOut>toTxOutonly to keep consistency with theCoinbaseRewardScriptwhere possible.In the config files for
Pool,JDS,JDCthecoinbase_outputwas renamed tocoinbase_reward_scriptdeprecate
Pool::get_coinbase_outputandJDS::get_txout