add solo mining mode to the pool app

[lucasbalieiro]

closes #135
replaces #269

this PR introduces a new pool configuration parameter: solo_mining_mode.

pool now validates every Open Channel message (standard or extended) to search for a specially formatted user_identity:

• Partial donation: sri/donate/reward_percentage/payout_address/worker_name
• Full donation: sri/donate/worker_name
• Solo: sri/solo/payout_address/worker_name
• Legacy Solo <valid_bitcoin_address>
• Normal assumes Pool mode if the user_identity does not fall in one of the previous category.

Added an offset to the computation of CoinbaseOutputConstraints

see: #288

[GitGab19]

I know it's still in draft, but I just want to recall that we should also support the usual case where the miner simply uses its Bitcoin address in the user_identity, without any specific sri/* option.

That case should be identified as SOLO (e.g. sri/solo/payout_address/worker_name).

[plebhash]

the scope of #325 is slightly different from what we're doing here, but looking at it made me think we should have some integration tests here as well

proposed coverage:

• full donation via pseudorandom user_identity (by pseudorandom I mean not following sri/ magic bytes nor legacy solo mining standard
• full donation via user_identity = "sri/donate" pattern
• partial donation via user_identity = "sri/donate/5/bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq" pattern
• solo via user_identity = "sri/solo/bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4" pattern

for each scenario, we leverage MockDownstream to craft a specific OpenExtendedMiningJob message with the desired user_identity

then we deserialize the coinbase output into a Transaction object (via rust-bitcoin APIs) and assert that the outputs match whatever was set on user_identity for that specific case

I don't think we need to involve Sv1/translation

[lucasbalieiro]

Let's Go! This is ready for review.

@GitGab19, added the support for the "legacy" format.

@plebhash, removed the new parameter solo_mining_mode and I already have a few tests covering your early comment, see 4390aba
Also adjusted the CoinbaseOutputConstraints problem

This probably still needs some cleanup from the early iteration that I'll address once I land

[GitGab19]

@lucasbalieiro I've tested all the possible variants of user_identity, and the behavior is handled properly.

The problem is that the outputs are correctly handled and used for the very first job only, but as soon as a second NewTemplate arrives, the whole reward goes to the Pool's address in the relative job created (e.g. we are not considering the previous request from the user_identity).

[plebhash]

@lucasbalieiro I've tested all the possible variants of user_identity, and the behavior is handled properly.

The problem is that the outputs are correctly handled and used for the very first job only, but as soon as a second NewTemplate arrives, the whole reward goes to the Pool's address in the relative job created (e.g. we are not considering the previous request from the user_identity).

commenting from mobile (not looking at the source)

but my guess is that we're only using user_identity on the handle_open_*_mining_channel but not on handle_new_template

[lucasbalieiro]

@lucasbalieiro I've tested all the possible variants of user_identity, and the behavior is handled properly.
The problem is that the outputs are correctly handled and used for the very first job only, but as soon as a second NewTemplate arrives, the whole reward goes to the Pool's address in the relative job created (e.g. we are not considering the previous request from the user_identity).

commenting from mobile (not looking at the source)

but my guess is that we're only using user_identity on the handle_open_*_mining_channel but not on handle_new_template

Yes! Exactly.

I'm currently doing a test on storing the PayoutMode in the DownstreamData and using it inside handle_new_template.

So we do the parsing and computing of the coinbase output only in the channel opening process. Than just re-use it

[GitGab19]

So we do the parsing and computing of the coinbase output only in the channel opening process. Than just re-use it

The problem is that in the handle_new_template we are not taking the PayoutMode from the DownstreamData.

[lucasbalieiro]

applied changes to handle_new_template here: 84c5294

I'm still working on a integration test for it. But, i think you guys can take a look at the approach

[GitGab19]

I'm still experiencing the same issue, and it's due to the fact that we're not passing the downstream_coinbase_outputs in the on_new_template().

I tested this fix and now it works perfectly.

[Shourya742]

Some suggestions, rest looks good.

[GitGab19]

tACK

[xyephy]

tACK
Reviewed and tested locally. Build passes, all 8 solo mining integration tests and 6 unit tests pass. The handle_new_template fix correctly checks data.payout_mode per downstream and passes downstream_coinbase_outputs to both group_channel.on_new_template() and standard_channel.on_new_template().

Some minor nits: -Add an integration test that waits for a second NewExtendedMiningJob after channel open and verifies the coinbase still pays the miner's address -- current tests only verify the first job.
-README line 91: OpenExtendMiningChannel -> OpenExtendedMiningChannel

[plebhash]

Some minor nits: -Add an integration test that waits for a second NewExtendedMiningJob after channel open and verifies the coinbase still pays the miner's address -- current tests only verify the first job.
-README line 91: OpenExtendMiningChannel -> OpenExtendedMiningChannel

the tests can be expanded to cover two events:

• mempool tx (which triggers new `NewTemplate)
• new block (which triggers new NewTemplate+SetNewPrevHash)

[Shourya742]

ACK

[lucasbalieiro]

Expanded the tests scope to validate the the outputs after a new NewExtendedMiningJob.

The CI is complaining about something outside of the scope of this PR. Do you guys wnat me to update it here?

[plebhash]

Expanded the tests scope to validate the the outputs after a new NewExtendedMiningJob.

The CI is complaining about something outside of the scope of this PR. Do you guys wnat me to update it here?

should be fixed via #339

[plebhash]

Some minor nits: -Add an integration test that waits for a second NewExtendedMiningJob after channel open and verifies the coinbase still pays the miner's address -- current tests only verify the first job.
-README line 91: OpenExtendMiningChannel -> OpenExtendedMiningChannel

the tests can be expanded to cover two events:

• mempool tx (which triggers new NewTemplate)
• new block (which triggers new NewTemplate+SetNewPrevHash)

@lucasbalieiro it seems we're only covering the second scenario

please note that NewTemplate != NewTemplate+SetNewPrevHash

please do a tp.create_mempool_transaction() + assertion before the tp.generate_blocks(1)