URI-handling code update: added safety checks and notifications by Diapolo · Pull Request #1002 · bitcoin/bitcoin

Diapolo · 2012-03-28T13:21:28Z

validate bitcoin address in the URI before switching to sendCoinsPage and pasting into the form, when a bitcoin: link is clicked
validate bitcoin address in the URI before switching to sendCoinsPage and pasting into the form, when a bitcoin: link is dropped on the Bitcoin-Qt window (Drag&Drop feature)
show a tray-notification if an URI could not be parsed to alert the user

laanwj · 2012-03-28T14:22:09Z

Good idea.

I think there should also be a notification if an invalid URI is sent, to
aid web developers etc in testing (hey, where did my url silently go)
Op 28 mrt. 2012 15:21 schreef "Philip Kaufmann" <
reply@reply.github.com>
het volgende:

Current state of this request:

added:

validate bitcoin address in the URL before switching to sendCoinsPage
and pasting into the form, when a bitcoin: link is clicked

validate bitcoin address in the URL before switching to sendCoinsPage
and pasting into the form, when a bitcoin: link is dropped on the
Bitcoin-Qt window (Drag&Drop feature)

I verified both cases in my own build on Windows, which is based on RC5
master.

Dia

You can merge this Pull Request by running:

git pull https://github.com/Diapolo/bitcoin URL-handling_2

Or you can view, comment on it, or merge it online at:

#1002

-- Commit Summary --

initial re-work on URL-handling code

-- File Changes --

M src/qt/bitcoingui.cpp (22)
M src/qt/guiutil.cpp (5)
M src/qt/sendcoinsdialog.cpp (12)
M src/qt/sendcoinsdialog.h (2)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/1002.patch
https://github.com/bitcoin/bitcoin/pull/1002.diff

Reply to this email directly or view it on GitHub:
#1002

Diapolo · 2012-03-28T19:41:21Z

I'm going to add what you suggested :), sounds good.

Diapolo · 2012-03-28T21:28:05Z

I added message boxes. A hint would be nice, if I should call "it" URL or URI ;)?
I'm also working on somewhat "hardening" the IPC code and I would prefer one of the two for every source comment, var name, function and so on.

Edit: I will take URI in the message boxes and where a user can see it and retain the current naming and not change code, where not needed! Will update the strings tomorrow...

laanwj · 2012-03-29T05:23:45Z

Nitpick: use notificator, not a message box

Re: URI or URL, I'm fine with either. Though URI is the most correct, more users know what URL means.

Diapolo · 2012-03-29T05:30:15Z

Cool idea with the notificator, will see how that looks :).

Diapolo · 2012-03-29T05:43:27Z

Changed to notificator, which is much better/smoother in terms of usability and usage flow.

laanwj · 2012-04-02T19:04:59Z

Can you please rebase this into one commit?

Diapolo · 2012-04-02T19:27:09Z

Done :).

laanwj · 2012-04-04T06:32:48Z

src/qt/bitcoingui.cpp

I'd use the exact same message here as on line 752 as there is no real need to distinguish between "URI handling and URI handling - drag&drop", and no one drags more URLs at a time. Saves the translators some work.

Agreed :). Thanks for your valuable suggestions.

Edit: Is it possible to paste more than 1 URI at all? I'm asking with the foreach in my mind.

luke-jr · 2012-04-06T16:34:45Z

src/qt/bitcoingui.cpp

Diapolo · 2012-04-06T20:40:43Z

Rebase to current master and all remaining URL / url were re-named to URI / uri ;).

luke-jr · 2012-04-10T13:51:12Z

src/qt/guiutil.cpp

These two need to be changed to URI to compile.

Diapolo · 2012-04-10T17:39:22Z

Rebased once more and fixed the 2 glitches luke observed :).

sipa · 2012-04-20T14:14:22Z

What does @TheBlueMatt think about this?

TheBlueMatt · 2012-04-20T20:03:00Z

src/qt/bitcoingui.cpp

I would think it would be better to handle any valid URIs and ignore invalid ones if there are valid ones present, but I dont really think many OSs will give a list of URIs unless you are dragging a folder full of files, and in that case, none of them will be bitcoin:, so it probably doesnt matter.

TheBlueMatt · 2012-04-20T23:34:22Z

Those are minor gripes anyway, ACK

Diapolo · 2012-04-21T17:25:05Z

@TheBlueMatt I'll look into your suggestions, even small glitches can be changed / fixed :).

Diapolo · 2012-04-29T17:58:42Z

Rebased once more, merged the 3 commits into a single one and re-worked a few days ago to include one of @TheBlueMatt suggestions.

Diapolo · 2012-05-08T20:50:36Z

Rebased and reworded the commit message!

Diapolo · 2012-05-12T22:20:42Z

Rebased and fixed merge-conflict. Can this get in please, if there are no further wishes?

laanwj · 2012-05-13T07:39:47Z

I think this one's waiting for URI support to be resurrected. Otherwise, it's kind of hard (and insensible) to test.

Diapolo · 2012-05-13T10:33:57Z

At least for Linux URI-support is in the client ;). And it's working fine with my build ^^, just wanted to bring this back into devs-mind.

TheBlueMatt · 2012-05-20T13:57:35Z

I agree with @Diapolo here, we have a huge list of pulls piling up (dont we always...) and I see little reason to not pull this because URIs are supported, just not on Windows or Mac.

Diapolo · 2012-05-24T17:57:56Z

If this would get in, I didn't need to rebase and keep this current, so ... :).

Diapolo · 2012-05-27T15:01:22Z

Updated to use toggleHidden() function for showing the window after a bitcoin-URI was clicked.

Diapolo · 2012-06-02T13:28:25Z

Rebased to fix a merge-conflict.

Diapolo · 2012-06-13T14:02:47Z

Rebased and changed back the use of toggleHidden() into showNormalIfMinimized() as another pull takes care of this. Can we please merge this with #1437!

Diapolo · 2012-06-14T06:01:08Z

Updated to fix a merge conflict.

URI-handling code update: added safety checks and notifications

[Phase1] Introduce shared pointers for transactions: Make CBlock a vector of shared tx pointers

4ea0476 [Consensus] Fix difficulty adjustment on first block of timeproto V2 (random-zebra) c254df6 [P2P] Do not store more than 200 timedata samples. (random-zebra) 8ca3979 [Tests] (squashed) few regtest fixes (random-zebra) 3487e58 [PoS] Fix Stake maturity-check and mapHashedBlocks time (random-zebra) 55aeecd [Core] raise nTargetTimespan_V2 to 30 minutes (random-zebra) 79bdc83 [Core] Restore CheckBlock checks before previous block check (random-zebra) 3bb0c0a [PoS] Don't use chainActive.Tip() as prevIndex when creating new blocks (random-zebra) 55faec1 [Performance] Kernel, methods using object reference instead of copy the object. (furszy) 4afcba5 [PoS] Don't allow consecutive blocks within the same time slot (random-zebra) c97e998 [PoS] StakeV1: iterate the time backwards. (random-zebra) 59cd3af [PoS] reduce difficulty by a factor 16 on nBlockTimeProtocolV2 (random-zebra) 3447dfb [Core] Make bnProofOfStakeLimit_V2 16 times v1 limit (random-zebra) 0bcf0c1 [PoS] Keep v1 miner until hard-fork (random-zebra) ec45670 [Core] Guard the transition to v2 time protocol in MinPastBlockTime rule (random-zebra) fee5f5c [PoS] set nTargetTimespan_V2 to 15 minutes (60 time slots) (random-zebra) ca2b035 [PoS] Set target spacing to 60 seconds (random-zebra) 0578b0a [PoS] Adjust FutureBlockTimeDrift to 15 secs granularity (random-zebra) dd71075 [Core] Set time granularity to 15 (instead of 16) (random-zebra) 8c3546f [Consensus] Change difficulty computation for V2 time protocol (random-zebra) f892e45 [Core/PoS] Enforce timestamp mask in miner (random-zebra) cdaf818 [Core] Define new Past Time Limit and enforce masked time (random-zebra) bcb5836 [Core] Define new Future Time Drift (random-zebra) 159eda9 [Cleanup] Remove unused function CheckCoinStakeTimestamp (random-zebra) 9773c09 [Core] Add timestamp mask to chainparams (random-zebra) 1d8fb33 [COPY] Update missing headers (random-zebra) 6f09121 [Consensus] Add placeholder block height for Time Protocol V2 (random-zebra) Pull request description: Main changes: - **granularity**: 15 secs, instead of 1 - **future limit**: adjustedTime + 14 secs (current timeslot) instead of 3 mins - **past limit**: prevBlock time - **target timespan**: 15 mins (instead of 40) ACKs for top commit: furszy: Great, ACK 4ea0476 Mrs-X: ACK PIVX-Project@4ea0476 Tree-SHA512: a365297ba169f78ad1c30cc2f318dce09d6e98038a43d35c645814923829e793147e7fdbcbb3f701878a1f5f4892f468f19451fcacc25973db63c92c02e40aa1

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

Given the strict timestamp checks enforced with POS TimeProtocol v2 (introduced with bitcoin#1002: a block cannot have a timestamp earlier than previous blocks and cannot be more than 15 seconds in the future), we don't need the complex (and error-prone) logic of ComputeTimeSmart. We can, instead, simply rely on the blocktime, which already ensures a proper ordering for the transactions.

0c581fb [GUI] Update the record time when wtx.nTimeSmart changes (random-zebra) b1dc5d3 [Refactor] Encapsulate ComputeTimeSmart in CWalletTx (random-zebra) db99c41 [Cleanup] Remove unneeded GetComputedTxTime (random-zebra) 3f31b4f [Bug] Fix nTimeSmart computation (random-zebra) Pull request description: Given the strict timestamp checks enforced with POS TimeProtocol v2 (introduced with bitcoin#1002: a block cannot have a timestamp earlier than previous blocks and cannot be more than 15 seconds in the future), we don't need the complex (and error-prone) logic of the current implementation of `ComputeTimeSmart`. We can, instead, simply rely on the blocktime, which already ensures a proper ordering for the transactions. This also allows us to encapsulate the function in the `CWalletTx` class (renaming it to `UpdateTimeSmart`). We can also get rid of the additional `GetComputedTxTime` (with its additional LOCK, and main chain lookup), and just use `GetTxTime`. Finally, update the nSmartTime when a mempool transaction is added to a block. This should, hopefully, fix bitcoin#1346 (further testing needed). ACKs for top commit: furszy: ACK 0c581fb . Fuzzbawls: ACK 0c581fb Tree-SHA512: 6d2bed1fafa7a658bd42ff2f7fe003087de368e2f49088104bce2409e4370caee0e1a153dc519525d7647b7afa1322fa1be968eefbd299404e1c8a521e24023e

laanwj reviewed Apr 4, 2012
View reviewed changes

laanwj mentioned this pull request Apr 4, 2012

modified block DL progressbar to be more informative and precise #1025

Merged

luke-jr reviewed Apr 6, 2012
View reviewed changes

src/qt/bitcoingui.cpp Outdated

Copy link
Copy Markdown

Member

luke-jr Apr 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uri!

luke-jr reviewed Apr 10, 2012
View reviewed changes

src/qt/guiutil.cpp Outdated

Copy link
Copy Markdown

Member

luke-jr Apr 10, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two need to be changed to URI to compile.

TheBlueMatt reviewed Apr 20, 2012
View reviewed changes

URI-handling code update: added safety checks and tray-notifications

93b7af3

laanwj added a commit that referenced this pull request Jun 14, 2012

Merge pull request #1002 from Diapolo/URL-handling_2

64d46e7

URI-handling code update: added safety checks and notifications

laanwj merged commit 64d46e7 into bitcoin:master Jun 14, 2012

coblee referenced this pull request in litecoin-project/litecoin Jul 17, 2012

Merge pull request #1002 from Diapolo/URL-handling_2

470797b

URI-handling code update: added safety checks and notifications

ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Apr 4, 2018

Merge pull request bitcoin#1002 from ptschip/dev_txref

8430e4f

[Phase1] Introduce shared pointers for transactions: Make CBlock a vector of shared tx pointers

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

Diapolo commented Mar 28, 2012

Uh oh!

laanwj commented Mar 28, 2012

Uh oh!

Diapolo commented Mar 28, 2012

Uh oh!

Diapolo commented Mar 28, 2012

Uh oh!

laanwj commented Mar 29, 2012

Uh oh!

Diapolo commented Mar 29, 2012

Uh oh!

Diapolo commented Mar 29, 2012

Uh oh!

laanwj commented Apr 2, 2012

Uh oh!

Diapolo commented Apr 2, 2012

Uh oh!

laanwj Apr 4, 2012

Choose a reason for hiding this comment

Uh oh!

Diapolo Apr 4, 2012

Choose a reason for hiding this comment

Uh oh!

luke-jr Apr 6, 2012

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Apr 6, 2012

Uh oh!

luke-jr Apr 10, 2012

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Apr 10, 2012

Uh oh!

sipa commented Apr 20, 2012

Uh oh!

TheBlueMatt Apr 20, 2012

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Apr 20, 2012

Uh oh!

Diapolo commented Apr 21, 2012

Uh oh!

Diapolo commented Apr 29, 2012

Uh oh!

Diapolo commented May 8, 2012

Uh oh!

Diapolo commented May 12, 2012

Uh oh!

laanwj commented May 13, 2012

Uh oh!

Diapolo commented May 13, 2012

Uh oh!

TheBlueMatt commented May 20, 2012

Uh oh!

Diapolo commented May 24, 2012

Uh oh!

Diapolo commented May 27, 2012

Uh oh!

Diapolo commented Jun 2, 2012

Uh oh!

Diapolo commented Jun 13, 2012

Uh oh!

Diapolo commented Jun 14, 2012

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants