Details
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- sm3
- Lint
Lint Passed Severity Location Code Message Auto-Fix src/net_processing.cpp:1 CFMT Code style violation Auto-Fix src/validation.cpp:1 CFMT Code style violation - Unit
No Test Coverage - Build Status
Buildable 2618 Build 3350: Bitcoin ABC Buildbot (legacy) Build 3349: arc lint + arc unit
Event Timeline
src/validation.cpp | ||
---|---|---|
4042 ↗ | (On Diff #4149) | Good catch. This was necessary in prior versions before you suggested I make modifications to the work comparator. It's no longer necessary. |
src/net_processing.cpp | ||
---|---|---|
923 ↗ | (On Diff #4149) | Fair. I'll remove it. |
src/net_processing.cpp | ||
---|---|---|
923 ↗ | (On Diff #4149) |
src/validation.cpp | ||
---|---|---|
3887 ↗ | (On Diff #4264) | Please keep fHasMoreWork and then you can add: bool chainTipShouldUpdate = isSameHeightAndMoreHonestlyMined || fHasMoreWork; Keeping names for intermediate variable makes the code more readable. |
3922 ↗ | (On Diff #4264) | This comment is invalid now. |
3944 ↗ | (On Diff #4264) | Considering this doesn't trigger when fHasMoreWork is true, then what ensures a reorg happens ? Is calling NewPoWValidBlock really the right thing to do ? After all, the chain we want to pick doesn't have more PoW behing it, just more accurate timestamps. |
test/functional/sendheaders-selfish-mining.py | ||
1 ↗ | (On Diff #4264) | The whole test is very confusing because way too much focused on selfish mining and not ont he behavior is needs to test, namely that more accurate timestamp are preferred. You should rewrite it in term of block with accurate/inaccurate timestamp. Tests cases: 1/
2/
3/ Ensure that 1 and 2 also work when the fork is more than one level deep. It relies on sleep which is generally a bad idea, especially since it's fairly easy to spoof timestamps and mock the timestamp of the node. |
43 ↗ | (On Diff #4264) | Please give it a more descriptive name. You already boostrapped 2 nodes and this is the 3rd one, so surely, it's something more specific than just THE node. |
75 ↗ | (On Diff #4264) | You cannot really rely on this. But you can setmocktime the node. You can also abuse the time difference (for instance 1h) to make the whole thing very reliable. |
81 ↗ | (On Diff #4264) | Please assign this before using it. It makes the code easier to follow. |
Still need to fix the test, but this is currently deep in my backlog. Will get to it whenever I can.
src/validation.cpp | ||
---|---|---|
3922 ↗ | (On Diff #4264) | It's still valid because less-work is < not <=. It would have been more accurate in the past if it said same-or-less work. |
3944 ↗ | (On Diff #4264) | I think the naming here is confusing. This hooks into the net_processing code that announces new blocks. If this node receives a more honest block, I think it makes sense to announce that block ASAP or else we're encouraging the honest block to be orphaned by not sharing it with the network as fast as possible. |
3948 ↗ | (On Diff #4149) | This doesn't seem right. Why show this log message for regular chaintip updates? This log message is only intended for when an apparent selfish mining operation is occurring. |
Fixed according to feedback, except Test 1.b fails. It really should not or could encourage a chain split. I'm yet sure why the longest PoW on the previously less-accurate timestamped block is not overtaking the less PoW chain.
src/validation.cpp | ||
---|---|---|
3506 ↗ | (On Diff #5505) | Sounds like you should use the comparator itself. |
3511 ↗ | (On Diff #5505) | The reorg will be in the log anyways. |
3541 ↗ | (On Diff #5505) | My understanding is that this is a simple optimization. The code would work just as fine without it, but if there is no reason to consider the block for reorg, then we can stop here and reconsider later. While this is the correct course of action, however it is not clear to me what triggers the downloads and/or redownload of blocks. If the code doesn't work when this is left alone, then this is mostly likely indication that something is lurking deeper. |
3569 ↗ | (On Diff #5505) | Why is that necessary ? It looks like it is changing the semantic completely. |
3/ Ensure that 1 and 2 also work when the fork is more than one level deep.
Addressing this since it isn't part of the tests: I did not intend for this to work beyond 1 block deep.
src/validation.cpp | ||
---|---|---|
3506 ↗ | (On Diff #5505) | Good idea. |
3511 ↗ | (On Diff #5505) | While true, I thought it was important to indicate why, since this is a rarer case than normal. |
3541 ↗ | (On Diff #5505) | iirc it does work without this change. I'll test again and confirm. |
3569 ↗ | (On Diff #5505) | While not strictly necessary, I figured broadcasting selfish-looking blocks would not be useful. To be clear, it does change the semantic. |
src/validation.cpp | ||
---|---|---|
3541 ↗ | (On Diff #5505) | Which means there is a problem somewhere that needs to be identified. |
src/validation.cpp | ||
---|---|---|
3569 ↗ | (On Diff #5505) | It will boradcast all block that do not immediately build on the tip now. |
src/validation.cpp | ||
---|---|---|
3541 ↗ | (On Diff #5505) | p2p-fullblocktest fails without this change, so I'll keep it as is. |
src/validation.cpp | ||
---|---|---|
3569 ↗ | (On Diff #5505) | The comment above this suggests the opposite. NewPoWValidBlock(), which broadcasts compact blocks to peers, is only called for more timestamp-accurate or longer chainwork blocks. |
Regarding this: I noticed in a recent reddit discussion that ProHashing does not use accurate timestamps: https://www.reddit.com/r/btc/comments/bqy05r/sensible_defaults_and_unintended_consequences_of/eo9f9fa/
If this is seriously planned to be landed, then it would be good to actually talk to miners and see what they think.
With regards to this comment and others I've heard from miners, it's clear to me that a change like this has much more risk than initially thought and does not provide enough benefit to offset that risk. This change is also not being maintained at all, so I think it's time to abandon it.