test multiple coinbase outputs round-trip - #325
Conversation
1706f1c to
1a2b6db
Compare
1a2b6db to
7b4e11f
Compare
7b4e11f to
5605a41
Compare
5605a41 to
6e5254a
Compare
|
How is this different wrt integration tests we added in #321? Aren't we already testing multiple coinbase outputs in those tests? |
No — #321 checks that the pool correctly maps |
|
I see. Are you gonna solve conflicts and make this PR up to date? |
yes |
2092a48 to
1acda25
Compare
There was a problem hiding this comment.
I don't see how the tests on this file are asserting what this PR aims to achieve.
we simply assert that some block was mined, while there's no assertion with regards to how many outputs there are in the coinbase tx
There was a problem hiding this comment.
Fair point, block acceptance alone is too weak. I will replace it with a block-inspection assertion (decode coinbase tx, check count + witness commitment + reward).
| // Requires a custom Bitcoin Core build with -testmulticoinbase support, which adds extra | ||
| // OP_RETURN outputs to the coinbase. Set BITCOIN_NODE_BIN to the custom binary path. | ||
| #[tokio::test] | ||
| #[ignore = "requires custom Bitcoin Core build with -testmulticoinbase; set BITCOIN_NODE_BIN"] |
There was a problem hiding this comment.
the strategy behind this ignore is not clear
There was a problem hiding this comment.
what does one need to do to get testmulticoinbase into Bitcoin Core?
and if it's not readily available on official Bitcoin Core distribution, why should it be covered by SRI CI?
There was a problem hiding this comment.
You would need a local Bitcoin Core patch since there's no upstream path. I patched it locally when I was working on stratum-mining/sv2-tp#55, and the upstream PR that would have made it testable without a patch, bitcoin/bitcoin#33890, was closed unmerged.
Agreed it shouldn't be in SRI CI. Dropping the test. Looking for an SRI-compatible alternative for issue #58, will propose before re-pushing.
There was a problem hiding this comment.
Given that the idea proposed in bitcoin/bitcoin#33890 wasn't useful for Sv2, and that it got closed, I'm wondering if issue #58 makes sense at all at this point.
As I was already pointing out in this comment we already have a test which covers the case of having multiple outputs in a coinbase tx, it's called pool_solo_mining_partial_donation().
Maybe the thing which is missing there is simply the block submission, which could be eventually added very easily without adding anything else.
1acda25 to
f0396f7
Compare
|
Updated: dropped the |
f0396f7 to
76c9c83
Compare
3 participants
Add an end-to-end test: mine a real block through TP → Pool → Translator → Minerd, then assert the accepted block's coinbase outputs and that it includes a real mempool transaction.
Closes #58