-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Getting this error as of today #62
Comments
Oh. Need to write an additional transaction handler for MWEB. |
AFAIK p2pool does not need the parsed MWEB transaction data for anything. P2pool needs to be able to parse transactions that are in block templates because p2pool does block assembly (it needs to be able to trim off transactions to avoid exceeding the blocksize limit after adding a big coinbase tx), but IIUC the MWEB is just a hash that p2pool commits. Previous versions of p2pool (before #22) would propagate the transactions over the p2pool p2p layer in order to do fast block/share propagation between p2pool nodes, but that functionality was removed after Compact Blocks made it effectively obsolete. So we could probably get away with changing the handler for a tx msg over p2p to do nothing at all. This might cause issues for starting a new share chain from scratch, but we could probably solve that by removing support for older share versions (i.e. < 34). This might also improve performance, CPU load, and memory consumption. |
File "/root/p2poolSoloNew/p2pool/bitcoin/helper.py", line 102, in getwork This error occurs when parsing transactions after a call getblocktemplate |
Hmm, does getblocktemplate also return the pending MWEB transactions? |
I think so. This issue requires more careful study. |
I have been watching for a long time when this error appears. Upd. The error also appears with a full mempool. |
I'm not a programmer... but let me know if i can help somehow. |
Watching and waiting for updates on this issue and ready to assist with testing, logs, etc. Let me know if I can help. Thanks! |
"3. If your pool software modifies, reorders, or reprioritizes transactions that it receives from getblocktemplate, it must be updated to ensure it never alters the final transaction, known as the HogEx or "Integrating Transaction." The pool also must be careful to not modify or remove any transaction the HogEx depends on (txs with outputs it spends). The reason for (3) is that the HogEx commits to the MWEB data, and moves the pegged in coins to the HogAddr (address holding all MMEB coins), and sends the pegout coins to the addresses specified by the pegout transactions in the MWEB. Any mismatch in MWEB data and the HogEx transaction would result in the block not passing consensus checks." https://github.com/litecoin-project/litecoin/blob/v0.21.2/doc/mweb/mining-changes.md |
I read it. It is necessary to write and debug a new type of transaction handler. Unfortunately, I could not solve the issue quickly with the help of crutches. |
@kr1z1s our P2Pool development experience consists of a custom GUI only with no back end modifications. This problem is beyond our limited scope of knowledge of MWEB. We can assist with a bug bounty only. @northern-lites This is a continuation of issue #59. Can you assist? @jtoomim Is raising the minimum relay TX fee to keep the number of transactions below the crash threshold a possible workaround until this is fixed? Please advise. Thanks! |
I've not seen the error occur when there are 0 tx in mempool (ex: right after block solve). Nor have I seen this when there's just 1 tx in mempool (coinbase transaction) which you can try yourself by running litecoind with
I agree that this will require some deeper inspection.
This might help reduce occurrences of errors until actual fix is applied. |
Observation -- when p2pool logs throw error |
Suggested below. Before fixing problem with transactions, you can run litecoind with param --blocksonly I work for a MWEB handler. But the process is not fast. |
My current thinking is that p2pool does not need to decode transactions at all, and should simply handle transactions as a dict with the raw binary representation of the tx (or possibly the hex, not sure), the transaction fee, and the size and weight. Rather than adding code to handle MWEB, we should remove the code that makes handling MWEB necessary. |
@mansilladev HyperDonkey.com's East US P2Pool node successfully merge mined a LTC and DOGE block today with the "--blocksonly" option enabled on Litecoind as suggested. A full log dump is attached for review.
@jtoomim This is a temporary workaround only. P2Pool is still unable to mine Litecoin MWEB blocks containing transactions. There seem to be conflicting ideas on how to handle P2Pool MWEB transactions. Can you clarify and provide some direction? David Burkett has also offered to help answer P2Pool related questions on the Litecoin MWEB Development Telegram here: @kr1z1s HyperDonkey.com is offering a 5,000 DOGE BUG BOUNTY for a tested and merged fix for this issue! (Bug Bounty reward address: D7qVwccq5Bmq4hoCG2kJMGsktTWAL4TnTS) |
Are you guys observing an actual crash in p2pool when this bug gets triggered, or is it just an error with a traceback which p2pool reports without fully crashing? My node (http://ml.toom.im:9327) has been running this code for weeks, and while I do frequently see this error, I haven't seen my node go down at all. |
The only reason why p2pool might need to change the transactions in a block template is if the p2pool coinbase transaction is large enough that it would put the block over the e.g. 1 MB limit. A workaround for this might be to hardcode p2pool to not modify block templates on LTC and also configure litecoind to limit its block templates to e.g. 970 kB to allow for up to 30 kB for the coinbase transaction. (I've seen coinbase txs of around 10 kB before on the BTC p2pool, though the LTC p2pool usually has fewer users and payout addresses than that.) |
We were experiencing random P2Pool crashes and miner disconnections prior to adding "--blocksonly" to the Litecoind daemon. P2Pool seems to run without errors when no Litecoin transactions are included. There was only one transaction in the last Litecoin block with "--blocksonly" enabled. Is that the MWEB HogEx transaction? https://chainz.cryptoid.info/ltc/block.dws?2274836.htm I received a request from a HyperDonkey.com merge miner to donate to the existing 5,000 DOGE P2Pool Bug Bounty. All Dogecoin sent to this address (D7qVwccq5Bmq4hoCG2kJMGsktTWAL4TnTS) will be awarded to the coder(s) of a tested and merged fix for this issue. (Update: The bug bounty is now 9,200 DOGE.) Please let me know if I can help with testing, etc. I also have error logs before/after the "--blocksonly" change. Thanks! |
@AdieLaine The version numbers are no longer relevant on this branch. If I had been updating version numbers (which I simply have not bothered to do), this repository would currently be P2Pool v35, not v16. The code on p2pool.in is obsolete, and has been for about 5 years. Your other comments are also off the mark and aren't relevant to this thread. |
You took discussion aside. Current problem is IN transaction parsing!
When transactions are disabled, block is created normally. And it is normally accepted by coin network. Changes here - https://github.com/litecoin-project/litecoin/blob/0.21/src/primitives/transaction.h We process these transactions as segwit - https://github.com/jtoomim/p2pool/blob/master/p2pool/bitcoin/data.py#L117 Structure of mweb transactions is different - https://github.com/litecoin-project/litecoin/blob/0.21/src/mweb/mweb_transact.h That's the whole problem. And stop looking at the version. It is taken from "github" files. You can put any number. |
@AdieLaine You don't know what you're talking about and are wasting our time.
In Everything else you have mentioned is off base as well, and I don't want to waste time answering all of it. |
@AdieLaine We already know exactly what the problem is. The bottleneck here is that no programmers have taken the time to either (a) write a parser in p2pool for the MW transaction format, or (b) change the p2pool code to no longer try to parse transactions and to treat them as simple binary blobs. Your guesses are unnecessary and wrong. |
@kr1z1s are you working on a fix? The bug bounty for this issue is up to 9,200 DOGE. I would like to promote it on social media to facilitate a resolution, but don't want to cause havoc if you're working on something already. Please advise. |
I'm working on something. |
https://github.com/jtoomim/p2pool/tree/rawtx I successfully made a few testnet blocks with this code. However, I haven't tried it on testnet with any transactions besides the default coinbase and HogEx txs. It ought to be tested with non-SegWit, SegWit spends (i.e. txid != hash), and MWEB transactions before using it on mainnet, as it's possible that this commit might have a bug in the merkle root calculation code with some of those transaction types, and therefore create invalid blocks. Also ought to be tested with blockchains other than LTC (e.g. BCH, BTC). Added bonus: this commit appears to reduce memory consumption by about 50% (unparsed txs are more RAM-efficient than parsed ones), and probably is faster/uses less CPU too. |
@jtoomim - on the
Now, it didn't seem to go as far as saying it couldn't connect to the litecoind server, and in fact, one of my workers still got a share accepted while So, is that |
@mansilladev I changed the code in getblocktemplate to not parse transactions. I didn't change the code to not parse transactions received over *coin p2p, because (as of share version 34) p2pool no longer uses txs received over p2p for anything. But apparently the code still tries to parse them (though I didn't notice this in my own testing). That should be pretty easy to fix. In the mean time, this unhandled exception and error message probably doesn't do any harm. |
@jtoomim Been running this branch for most of the day with some hashpower load against it, and it appears stable when MW txs come into mempool (other than the exception/error message). That being said, there I did notice a bug when trying to run it from scratch (no existing shares in
|
Congratulations and thank you @jtoomim! Someone on the LTC P2Pool network mined block #2280057 today with transactions and MWEB successfully included: HyperDonkey.com will now update all 3 of its nodes to the rawtx branch and report any remaining issues here. |
I pushed a couple of new commits to My initial hopes about reducing memory usage with this commit appear to be ill-founded. It now looks like this code increases memory usage. Grumble. |
@jtoomim We successfully merge mined Litecoin and Dogecoin blocks today with the new commits to rawtx and no errors!
Please reply with your Dogecoin address so I can disburse the 9,200 DOGE bug bounty. Thank you and @mansilladev for supporting P2Pool! |
DDnEa13uArBr95ZFP8We4TYqLwezkV4tcu @Azskygod |
@jtoomim Confirmed. Bug bounty of 9,199.99363364 DOGE sent. Thank you! |
Hi guys,
And:
After a few minutes it goes back to normal. Thank you, |
@ikolubr The rawtx branch fixes that. |
Thank you. I thought you had merged all into master. ;) |
@ikolubr Not yet. The rawtx branch appears to be a regression for memory usage, so until that has been solved, I'm reluctant to merge it and worsen p2pool for non-LTC users. |
The text was updated successfully, but these errors were encountered: