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.

Sign up for GitHub

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

Jump to bottom

Refactor transaction/accounting time #1393

Merged
merged 3 commits into from
Aug 23, 2012
Merged

Refactor transaction/accounting time #1393

merged 3 commits into from
Aug 23, 2012

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented May 28, 2012

Status: Tested and seems to work

  1. Guarantee listtransactions order consistency by storing a position for each entry
  2. Add "blocktime" and (for wallet transactions) "timereceived" to transaction Objects
  3. Implement "smart" times according to the following logic:
    • If sending a transaction, assign its timestamp to the current time.
    • If receiving a transaction outside a block, assign its timestamp to the current time.
    • If receiving a block with a future timestamp, assign all its (not already known) transactions' timestamps to the current time.
    • If receiving a block with a past timestamp, before the most recent known transaction (that we care about), assign all its (not already known) transactions' timestamps to the same timestamp as that most-recent-known transaction.
    • If receiving a block with a past timestamp, but after the most recent known transaction, assign all its (not already known) transactions' timestamps to the block time.

This supercedes #1159.
Discussion: https://bitcointalk.org/?topic=54527

@gmaxwell
Copy link
Contributor

In spite of the non-determinism of the 'smart time' I like these criteria a lot.

@luke-jr
Copy link
Member Author

luke-jr commented Jun 19, 2012

So got around to testing... all my transactions are showing a smart time of 1 (01/01/1970 00:00). Guess I have some fixing to do.

@luke-jr
Copy link
Member Author

luke-jr commented Jun 21, 2012

Reminder: A||B is boolean in C++, not the first value that casts to true. :(

Fixed.

@luke-jr
Copy link
Member Author

luke-jr commented Jun 30, 2012

Fixed a bug and tested some more.

Two open issues:

  • Would be nice if sorting transactions by time in Bitcoin-Qt respected the fixed order; I think Qt does it right now, though :/
  • Probably during -rescan and initial blockchain catchup, the block time should be trusted even if the user has transactions dated after them? Opinions on this case (easy to test by sending a transaction before catching up)

@laanwj
Copy link
Member

laanwj commented Jul 17, 2012

ACK

Re:Qt, sorting transactions by time sorts the transactions by time, nothing else. Anything else would be confusing IMO. The only thing that can be sensibly changed is the 'default' order, when not explicitly sorting by anything. In the transaction details we can show all the different times.

@luke-jr
Copy link
Member Author

luke-jr commented Jul 17, 2012

@laanwj Except for extreme differences in the local clock, the assigned order from this patch is always chronological; but right now, sorting by time in Bitcoin-Qt has no logic behind the order of transactions with the same time.

@luke-jr luke-jr mentioned this pull request Aug 1, 2012
Closed
@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f45fa25bb9efb6f83409e71b972f07cce0eb4520 for binaries and test log.

@luke-jr luke-jr mentioned this pull request Aug 13, 2012
Closed
@gmaxwell
Copy link
Contributor

Where does this stand?

@luke-jr
Copy link
Member Author

luke-jr commented Aug 23, 2012

Been running fine for me since next-test 2012-07

@luke-jr
Store a fixed order of transactions (and accounting) in the wallet
9c7722b 
For backward compatibility, new accounting data is stored after a \0 in the comment string.
This way, old versions and third-party software should load and store them, but all actual use (listtransactions, for example) ignores it.
@luke-jr
JSON-RPC: Add "blocktime" and (for wallet transactions) "timereceived…
bdbfd23 
…" to transaction Object outputs
@luke-jr
Choose reasonable "smart" times to display for transactions
c3f95ef 
Logic:
- If sending a transaction, assign its timestamp to the current time.
- If receiving a transaction outside a block, assign its timestamp to the current time.
- If receiving a block with a future timestamp, assign all its (not already known) transactions' timestamps to the current time.
- If receiving a block with a past timestamp, before the most recent known transaction (that we care about), assign all its (not already known) transactions' timestamps to the same timestamp as that most-recent-known transaction.
- If receiving a block with a past timestamp, but after the most recent known transaction, assign all its (not already known) transactions' timestamps to the block time.
@gmaxwell
Copy link
Contributor

ACK, works for me, apparently works for other people.

gmaxwell added a commit that referenced this pull request Aug 23, 2012
@gmaxwell
Merge pull request #1393 from luke-jr/refactor_times
47753fa 
Refactor transaction/accounting time
@gmaxwell gmaxwell merged commit 47753fa into bitcoin:master Aug 23, 2012
@gavinandresen
Copy link
Contributor

Hindsight is always 20/20....
... but this pull didn't get sufficient testing/code review. I think a test plan should have been written and followed to try to catch the bugs earlier.

@luke-jr
Copy link
Member Author

luke-jr commented Sep 8, 2012

I agree on lack of testing. Unfortunately, the tests it included didn't cover enough (and I haven't thought of any way to actually test the problems found either).

@IngCr3at1on IngCr3at1on mentioned this pull request Sep 28, 2015
Open
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
@UdjinM6 @schinzelh
Fail to proceed in listsinceblock if provided blockhash can't be found (
6aaec3a 
bitcoin#1393)

If daemon crashes, it can't save latest block sometimes, so querying daemon
for presumably best/last hash would result in a list of all txes recognized by
this wallet as its own since genesis block which could be confusing,
to say at least. Same applies for typos etc. This should fix it.

Not sure why but such weird behaviour was the case since listsinceblock rpc was
initially introduced in Bitcoin 0.5 (Oct 5, 2011)
bitcoin@3a6e468
@achow101 achow101 mentioned this pull request Dec 26, 2017
Closed
@promag promag mentioned this pull request Mar 5, 2018
Closed
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 22, 2019
@gandrewstone
Merge pull request bitcoin#1393 from awemany/remove-wallet-ismine
816c2e6 
Remove old wallet_ismine.cpp
@jonatack jonatack mentioned this pull request Dec 13, 2019
Closed
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request May 6, 2020
@furszy
Merge bitcoin#1393: [GUI] Customizable sorting for address books
541a688 
c2f9f79 [Doc] Add GUI addressbooks sorting in the release notes (random-zebra)
5436a07 [GUI] Add sorting controls and connect them in addresseswidget (random-zebra)
77a8440 [GUI] Connect sort controls in coldstaking and receive widget (random-zebra)
51edff8 [GUI] Add controls to sort addresses in receive/coldstaking widget (random-zebra)
da23ce2 [GUI] Sort MyAddresses/Contacts/ColdStakingAddresses by label (random-zebra)

Pull request description:

  This adds GUI controls and relative logic to sort the addresses saved in the wallet and presented to the user.
  Interested views:
  - Receive --> My Addresses
  - Cold Staking --> My Cold Staking Addresses
  - Contacts
  - Send / Delegate --> recipient dropdown

  Addresses can now be sorted (ascending or descending order):
  - by label (default)
  - by address
  - by creation date.

  Closes bitcoin#1038 .

  ---

  For "receive" addresses and own "cold-staking" addresses, the controls are hidden with the list, and show up when the relative button is pressed:
  <br>

  ![sort_addys_1b](https://user-images.githubusercontent.com/18186894/76258309-992ea500-6253-11ea-974a-9b759a020a83.gif)

  ---

  <br>
  For "contacts" the controls are visible next to the title
  <br>

  ![sort_addys_2](https://user-images.githubusercontent.com/18186894/76258332-a3e93a00-6253-11ea-896b-745e21a71873.gif)

  ---

  <br>
  For the dropdown in the "send" widget, instead, a fixed by-label ascending order is used (adding controls for customization here would be probably useless and only make the the space overcrowded).
  <br>

  ![sort_addys_3](https://user-images.githubusercontent.com/18186894/76258372-b3688300-6253-11ea-9483-0ff175378fb3.gif)

  ---
  <br>

  Note: currently based on top of:
  - [x] bitcoin#1385

  The present diff starts with the second commit.

ACKs for top commit:
  furszy:
    ACK c2f9f79 .

Tree-SHA512: 7788b6d7eb7e1a966d2e1f0c0de95691d217571f440de1ec75732d7fd23a8251ea4cfbe37163d650bd2eb591ad8115affb1843c62253fe501540a34b93e91d86
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@luke-jr @gmaxwell @laanwj @BitcoinPullTester @gavinandresen