Skip to content
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

Relay and accept transactions that personally benefit the user #1128

Closed
wants to merge 3 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 20, 2012

Not sure why I didn't submit this a long time ago, but it seems like obvious behaviour...

@sipa
Copy link
Member

sipa commented Apr 20, 2012

I'm not sure it is a good idea to accept nonstandard (or otherwise non-policy compliant, whatever the policy is) transactions into your memory pool if you're not mining yourself.

@luke-jr
Copy link
Member Author

luke-jr commented Apr 20, 2012

Refusal to accept your own transactions breaks the rest of the wallet code right now, in my experience. At least this way, you can solicit a miner to accept it.

@sipa
Copy link
Member

sipa commented Apr 20, 2012

Of course, but the wallet shouldn't create such transactions. If it does, that's a bug.

@luke-jr
Copy link
Member Author

luke-jr commented Apr 20, 2012

Or a new feature in development.

So either it's a no-op in a non-buggy vanilla client, or it prevents bug compounding and makes testing new features easier.

@jgarzik
Copy link
Contributor

jgarzik commented Apr 20, 2012

Or (Satoshi would certainly say "no" here...) just add a cli switch that disables the isStandard test at runtime (i.e. accepts all valid transactions as standard), instead of hacking it half-way like this.

@luke-jr
Copy link
Member Author

luke-jr commented Apr 20, 2012

That'd be #559 ;)

I don't think bypassing the non-standard checks for ones own transactions is a half-way-hack.

@gavinandresen
Copy link
Contributor

Accepting transactions you don't understand is a really bad idea, ESPECIALLY if they are to you. Getting your client to accept a non-standard transaction that the rest of the network will eventually reject is a good first step to a successful double-spend.

No philosophical objection to the other commit, although I hate making the code more complicated for a "never actually a problem" case.

@luke-jr
Copy link
Member Author

luke-jr commented Apr 21, 2012

This doesn't accept non-standard transactions to you, only from you... or at least that's what I had in mind when I wrote it - does IsMine return true for ones you're receiving too?

@luke-jr
Copy link
Member Author

luke-jr commented Apr 22, 2012

Yeah, looks like I confused IsMine with IsFromMe. Closing until I've fixed this (let me know if it's wanted for 0.6.1 and I'll prioritize it)

@luke-jr luke-jr closed this Apr 22, 2012
@luke-jr luke-jr reopened this May 18, 2012
@luke-jr
Copy link
Member Author

luke-jr commented May 18, 2012

Rewrote, should fix the problems.

@jgarzik
Copy link
Contributor

jgarzik commented Aug 1, 2012

Sounds like consensus is fairly strong against accepting non-standard TX's, from yourself or not.

@jgarzik jgarzik closed this Aug 1, 2012
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
- make sure dsq is not too much in the future
- do not process seen dsq
- do not alter nTime for dsq localy, use memory only fTried instead
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 22, 2019
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Dec 25, 2019
f14af4f [Trivial] Fix timedata.cpp includes (random-zebra)
b6992ac [Tests] Add p2p_time_offset functional test to test_runner (random-zebra)
faf03a6 [Tests] Define p2p_time_offset functional test (random-zebra)
d9e9284 [Consensus] reduce possible nTimeOffset to 15 seconds (random-zebra)

Pull request description:

  Open for brainstorming.
  Follow up to bitcoin#1002

  - reject connection with a node having offset higher than 30 secs (and don't add the sample to timedata)
  - repeatedly trigger the warning message when the total offset (absolute value) is above 15 seconds (and set the offset to +15/-15 instead of 0).

ACKs for top commit:
  Fuzzbawls:
    utACK f14af4f
  Mrs-X:
    NIT that the last LogPrintf() does not have a time-unit anymore, but utACK PIVX-Project@f14af4f
  Warrows:
    ACK f14af4f

Tree-SHA512: e2a8b7ee7718814ce49fcb536af48afcf1b2829644699abbe5279b064efe1ff066c3100e65229abf06a7787b9f6e813b971300404841a0e6fe41375780a5e318
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Dec 25, 2019
44fc641 [P2P] Define CheckOffsetDisconnectedPeers (random-zebra)
21eb7dc [Bug] Use abs value of nTimeOffset to check whether to call AddTimeData (random-zebra)

Pull request description:

  Follow up to bitcoin#1128 .
  Fixes a bug where negative offsets would always lead to connection.

  Also addresses the concern raised about throwing a `uiInterface.ThreadSafeMessageBox` warning when the node isn't able to add any sample (due to excessive time drift).

  Logic:
  Node mantains a set `setOffsetDisconnectedPeers` of peer's IP addresses.
  When there is a disconnection due to excessive nTimeOffset, the peer's IP is added to the set, unless the node has already `ENOUGH_CONNECTIONS` (which is set to `2` intead of `1` to offer a bit of leeway).
  When the set contains `MAX_TIMEOFFSET_DISCONNECTIONS` (set to `16`) different IPs, the warning is triggered (and the set is cleared).

ACKs for top commit:
  furszy:
    utACK 44fc641
  Warrows:
    ACK 44fc641

Tree-SHA512: c49e4a62578820e1d125dd2b6a253f05a39b110d533b2083516c5d84410d665ad20f0415dffa2343db2ce4092bb253583a5cb12076f5c90180815cf961350096
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Dec 25, 2019
… back within range

c24d2b1 [Trivial] Clean time offset warning when it gets back within range (random-zebra)

Pull request description:

  Simple update (related to bitcoin#1128 and bitcoin#1138).
  Clear `strMiscWarning` if the median gets back within the offset limit (after being off, for example during startup when the node has very few time samples).

ACKs for top commit:
  Fuzzbawls:
    utACK c24d2b1
  furszy:
    utACK c24d2b1

Tree-SHA512: c0d4b206eb85cd6d6ba580a3e2b74d46b7518225733c88412cbf810f863910455b3daa35c72c9442d02a80593d79916f83eed26a2ca26903596f2a0182558906
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants