feat: Refactor OutPoint and add dep group support #1331

TheWaWaR · 2019-08-02T03:18:36Z

nervos-bot · 2019-08-02T03:18:41Z

@keroro520 is assigned as the chief reviewer

core/src/cell.rs

core/src/transaction.rs

doitian · 2019-08-03T02:51:27Z

protocol/src/protocol.fbs

    block_hash:     H256;
+    dep_type:       uint8;


Why not using a enum type here?

Maybe we should change Script.hash_type to enum as well.

I have a note regarding this here:

ckb/core/src/script.rs

Lines 11 to 15 in 59e7b47

// NOTE: we could've used enum as well in the wire format, but as of

// flatbuffer 1.11.0, unused constants will be generated in the Rust

// code for enum types, resulting in both compiler warnings and clippy

// errors. So for now we are sticking to a single integer in the wire

// format, and only use enums in core data structures.

shared/src/tx_pool/proposed.rs

core/src/cell.rs

core/src/transaction.rs

keroro520 · 2019-08-05T05:27:07Z

Previously we are allowed to specify block_hash in input-out-point, to ensure that the previous_output was contained in the given block.

I notice this PR remove block_hash from OutPoint, so that means we don't support this kind of inputs?

TheWaWaR · 2019-08-05T07:10:14Z

I notice this PR remove block_hash from OutPoint, so that means we don't support this kind of inputs?

Yes, we now should define this relationship in dep. dao is a good use case to learn this.

76e2a6b#diff-2d93f2ba5649afea4f5fbe0659022793R209

util/jsonrpc-types/src/blockchain.rs

BREAKING CHANGE: OutPoint changed, add CellDep data structure

BREAKING CHANGE: Transaction/OutPoint data structure changed

BREAKING CHANGE: Protocol schema changed

BREAKING CHANGE: schema changed

u2 · 2019-08-08T08:00:26Z

core/src/transaction.rs

+            let tx_hash = H256::from_slice(&data[tx_hash_start..tx_hash_end])
+                .expect("Invalid tx hash length");
+            let mut index_bytes = [0u8; mem::size_of::<u32>()];
+            index_bytes.copy_from_slice(&data[index_start..index_end]);


index_bytes.copy_from_slice(&data[index_start..]);

u2 · 2019-08-08T08:00:30Z

core/src/transaction.rs

+    /// Convert OutPoint to one dep group data item
+    pub fn to_group_data(&self) -> Vec<u8> {
+        [self.tx_hash.to_vec(), self.index.to_le_bytes().to_vec()].concat()
+    }


Please add some tests for the from/to conversion.

Changed to CellKey's implementation (see the code above in the same file).

u2 · 2019-08-08T08:33:07Z

core/src/cell.rs

@@ -31,7 +32,7 @@ impl BlockInfo {
 pub struct CellMeta {
    #[serde(skip)]
    pub cell_output: CellOutput,
-    pub out_point: CellOutPoint,
+    pub out_point: OutPoint,
    pub block_info: Option<BlockInfo>,
    pub cellbase: bool,


There is another struct TransactionInfo https://github.com/nervosnetwork/ckb/blob/develop/core/src/extras.rs#L19
It's similar to the BlockInfo, it seems that we can remove the BlockInfo, and we can remove the pub cellbase: bool as well, I think whether it's a cellbase depends on the position in the Block. https://github.com/nervosnetwork/ckb/blob/develop/store/src/store.rs#L195

Maybe create a new issue for another PR.

u2 · 2019-08-08T09:08:17Z

core/src/cell.rs

    Dead(OutPoint),
    Unknown(Vec<OutPoint>),
    OutOfOrder(OutPoint),
 }

+pub fn parse_dep_group_data(data: Bytes) -> Result<Vec<OutPoint>, usize> {
+    // tx hash (32 bytes) + output index (4 bytes)
+    const OUT_POINT_LEN: usize = mem::size_of::<H256>() + mem::size_of::<u32>();


put it in transaction.rs?

TheWaWaR · 2019-08-08T09:27:37Z

Close this by: #1356

nervos-bot bot assigned keroro520 Aug 2, 2019

TheWaWaR force-pushed the dep-group-support branch from 7c8230e to 399cc9b Compare August 2, 2019 09:39

doitian requested changes Aug 3, 2019

View reviewed changes

TheWaWaR commented Aug 3, 2019

View reviewed changes

shared/src/tx_pool/proposed.rs Outdated Show resolved Hide resolved

keroro520 suggested changes Aug 5, 2019

View reviewed changes

core/src/cell.rs Show resolved Hide resolved

core/src/transaction.rs Show resolved Hide resolved

TheWaWaR force-pushed the dep-group-support branch from df0584f to 2704bd5 Compare August 5, 2019 17:19

doitian requested changes Aug 6, 2019

View reviewed changes

util/jsonrpc-types/src/blockchain.rs Outdated Show resolved Hide resolved

TheWaWaR force-pushed the dep-group-support branch from 2704bd5 to 69204a7 Compare August 6, 2019 04:29

doitian requested changes Aug 6, 2019

View reviewed changes

util/jsonrpc-types/src/blockchain.rs Outdated Show resolved Hide resolved

TheWaWaR marked this pull request as ready for review August 6, 2019 06:18

TheWaWaR requested a review from a team August 6, 2019 06:18

TheWaWaR force-pushed the dep-group-support branch from 6894bcd to 1584d94 Compare August 6, 2019 09:21

TheWaWaR added 15 commits August 6, 2019 19:53

refactor: Change OutPoint, add CellDep, dep group support

665283d

BREAKING CHANGE: OutPoint changed, add CellDep data structure

test(core): Fix core tests

f733bda

refactor(jsonrpc-types): Update for OutPoint/CellDep

352b019

BREAKING CHANGE: Transaction/OutPoint data structure changed

refactor(protocol): Update for OutPoint/CellDep

f29afcc

BREAKING CHANGE: Protocol schema changed

refactor(protos): Update for OutPoint/CellDep

33c6e16

BREAKING CHANGE: schema changed

refactor(store): Change CellOutPoint to OutPoint

198b135

refactor: Update CellProvider/HeaderProvider

e04f4cf

refactor(script): Update for OutPoint/CellDep

047b29f

refactor(dao): Update for OutPoint/CellDep

69147db

refactor(shared): Update for OutPoint/CellDep

50e50e0

refactor(verification): Update for OutPoint/CellDep

f7f4de9

chore: Make clippy happy

2cd5418

test: Fix chain tests (in tx pool)

6bfc065

test: Update hashes

e2e20f1

test(rpc): Fix json rpc tests

bd3cce0

TheWaWaR added 6 commits August 6, 2019 19:58

test: Fix size_limit integration test

30313ee

chore: Impl Debug for CellDep

da89cde

test: Add test for dep group

5d8372e

fix: Tx pool handle dep group bug

f79e25c

chore: Do not skip cellbase input since verify

8ceec18

chore: Change json rpc CellDep data structure

76dd08c

TheWaWaR force-pushed the dep-group-support branch from 1584d94 to 76dd08c Compare August 6, 2019 12:30

keroro520 mentioned this pull request Aug 7, 2019

[HOLD] feat: create ckb-error #1344

Closed

u2 reviewed Aug 8, 2019

View reviewed changes

TheWaWaR closed this Aug 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor OutPoint and add dep group support #1331

feat: Refactor OutPoint and add dep group support #1331

TheWaWaR commented Aug 2, 2019 •

edited

Loading

nervos-bot bot commented Aug 2, 2019

doitian Aug 3, 2019

doitian Aug 3, 2019

xxuejie Aug 5, 2019

keroro520 commented Aug 5, 2019

TheWaWaR commented Aug 5, 2019 •

edited

Loading

u2 Aug 8, 2019

u2 Aug 8, 2019

TheWaWaR Aug 8, 2019

u2 Aug 8, 2019

TheWaWaR Aug 9, 2019

u2 Aug 9, 2019

u2 Aug 8, 2019

TheWaWaR commented Aug 8, 2019

	// NOTE: we could've used enum as well in the wire format, but as of
	// flatbuffer 1.11.0, unused constants will be generated in the Rust
	// code for enum types, resulting in both compiler warnings and clippy
	// errors. So for now we are sticking to a single integer in the wire
	// format, and only use enums in core data structures.

feat: Refactor OutPoint and add dep group support #1331

feat: Refactor OutPoint and add dep group support #1331

Conversation

TheWaWaR commented Aug 2, 2019 • edited Loading

nervos-bot bot commented Aug 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keroro520 commented Aug 5, 2019

TheWaWaR commented Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheWaWaR commented Aug 8, 2019

TheWaWaR commented Aug 2, 2019 •

edited

Loading

TheWaWaR commented Aug 5, 2019 •

edited

Loading