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: simpler read #11221

Merged
merged 1 commit into from Nov 9, 2017
Merged

Refactor: simpler read #11221

merged 1 commit into from Nov 9, 2017

Conversation

gnuser
Copy link
Contributor

@gnuser gnuser commented Sep 3, 2017

No description provided.

sipa
sipa reviewed Sep 3, 2017
View reviewed changes
src/streams.h
if (nReadPosNext > vch.size()) {
throw std::ios_base::failure("CDataStream::read(): end of data");
}
memcpy(pch, &vch[nReadPos], nSize);
Copy link
Member

@sipa sipa Sep 3, 2017 •
edited

Choose a reason for hiding this comment

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

This is not correct. When vch is empty, and nReadPos and nSize are 0, this will invoke operator[] on vch, which is not valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that that's a bug with the original code though, not with the refactor

Copy link
Member

@laanwj laanwj Sep 6, 2017 •
edited

Choose a reason for hiding this comment

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

probably better to use vch.data() + nNreadPos?
(but yes, this is not a bug introduced in this PR)

Copy link
Member

@promag promag Oct 8, 2017 •
edited

Choose a reason for hiding this comment

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

@sipa above there is an early return if nSize == 0 so that doesn't happen. Out of bounds access is prevented with the std::ios_base::failure exception.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, there is no bug here. It would still be clearer using vch.data() + nReadPos though.

@promag
Copy link
Member

promag commented Oct 8, 2017

utACK 9db9d62.

Nit, commit message could mention CDataStream::read instead read only.

promag
promag reviewed Oct 8, 2017
View reviewed changes
src/streams.h
throw std::ios_base::failure("CDataStream::read(): end of data");
}
memcpy(pch, &vch[nReadPos], nSize);
if (nReadPosNext == vch.size())
Copy link
Member

Choose a reason for hiding this comment

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

Nit, { on this line.

@laanwj laanwj merged commit 9db9d62 into bitcoin:master Nov 9, 2017
laanwj added a commit that referenced this pull request Nov 9, 2017
@laanwj
Merge #11221: Refactor: simpler read
0dec4cc 
9db9d62 Refactor: make the read function simpler (gnuser)

Pull request description:

Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
@jnewbery jnewbery mentioned this pull request Nov 15, 2017
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11221: Refactor: simpler read
0f978ba 
9db9d62 Refactor: make the read function simpler (gnuser)

Pull request description:

Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 31, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11221: Refactor: simpler read
fe38d4f 
9db9d62 Refactor: make the read function simpler (gnuser)

Pull request description:

Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 4, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11221: Refactor: simpler read
e213760 
9db9d62 Refactor: make the read function simpler (gnuser)

Pull request description:

Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 9, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#11221: Refactor: simpler read
2a018f8 
9db9d62 Refactor: make the read function simpler (gnuser)

Pull request description:

Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
@furszy furszy mentioned this pull request Sep 27, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Sep 27, 2020
@random-zebra
Merge #1875: BugFix: tx sync parsing problem fixed + extra refactoring.
8e7fa72 
9d8b8a2 BugFix: CMutableTransaction following the same sapling nVersion rules as CTransaction. (furszy)
1ca1b89 PeerLogicValidation missing destructor declaration warning fix. (furszy)
093a766 Introduce convenience type CTransactionRef (furszy)
8345a87 Refactor: make the read function simpler (gnuser)

Pull request description:

  PR solving the current master's syncing issue (syncing process not passing through block 5840), solved in 9d8b8a2.

  Essentially, when we merged #1815, we moved from using the default transaction constructor and the ser/unser template methods (`SerializationOp`) to be using, inside the templated serialization puzzle, the deserializing constructor (`CTransaction(deserialize_type, Stream& s)`) which internally creates a `CMutableTransaction` which wasn't having the same sapling tx version guard as `CTransaction` ser/unser method. So, in other words, it was trying to parse the shielded transaction data from an old version two transaction (yes, we already have version two transaction in our network.. first one is in block 5840).

  Plus, i took the mischief of not only including the bugfix, have added:

  * stream::read function readability improvement coming from bitcoin#11221.
  * a pretty straightforward adaptation of upstream's b4e4ba4  (missing last commit not included in #1815).
  * a `PeerLogicValidation` class compiler warning fix 1ca1b89

ACKs for top commit:
  random-zebra:
    ACK 9d8b8a2
  Fuzzbawls:
    ACK 9d8b8a2

Tree-SHA512: ffb60001160d177d20da3d9198f73910921ec521e6cae1ccbf9f6359dc96e3e4fdbbcaabb85331ce2a8b839c07ab34b3981c69d83aa6990aa79e134588539f48
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 30, 2021
@laanwj @gades
Merge bitcoin#11221: Refactor: simpler read
a3194e3 
9db9d62 Refactor: make the read function simpler (gnuser)

Pull request description:

Tree-SHA512: 5a80cc1b841488323d421e6a40b245d149cab1988247aed6cc7468dcc042d3df15b6711f25e40ff16e03ac21de36adbaa1d8da61ccdb94f97c8b70c24a5eedc5
@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

@laanwj laanwj laanwj left review comments

@sipa sipa sipa left review comments

@meshcollider meshcollider meshcollider left review comments

@promag promag promag left review comments

@dcousens dcousens dcousens approved these changes

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@gnuser @promag @laanwj @dcousens @sipa @meshcollider