π aguycalled opened a pull request: "Create wallet from seed"
(https://github.com/bitcoin/bitcoin/pull/30606)
This PR adds the option to create a wallet from a seed. The seed of a wallet can be obtained using the RPC command `getblsctseed`.
Two options to create a wallet:
- Using the `navio-wallet` tool together with the option `seed`. Example:
`navio-wallet -blsct -chain=blsctregtest -wallet=wallet_name -seed="71187dc33c03914c630e87b205777d51f7bd2749bc1164795e0134544d43d9ee" create`
- Using the rpc command `createwallet`. Example:
`navio-cli --blsctregtest -named createwallet wallet_name=walle
...
(https://github.com/bitcoin/bitcoin/pull/30606)
This PR adds the option to create a wallet from a seed. The seed of a wallet can be obtained using the RPC command `getblsctseed`.
Two options to create a wallet:
- Using the `navio-wallet` tool together with the option `seed`. Example:
`navio-wallet -blsct -chain=blsctregtest -wallet=wallet_name -seed="71187dc33c03914c630e87b205777d51f7bd2749bc1164795e0134544d43d9ee" create`
- Using the rpc command `createwallet`. Example:
`navio-cli --blsctregtest -named createwallet wallet_name=walle
...
π¬ fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707958169)
See the description: "The first commit shows how to reproduce the error shown in https://github.com/bitcoin/bitcoin/issues/30514 and how it could be fixed with a minimal change. The second commit adds an explicit sanitizer that will probably be the preferred approach. I can squash the commits but first I would like to hear conceptual feedback since https://github.com/bitcoin/bitcoin/issues/30514 suggested to remove the block height instead."
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707958169)
See the description: "The first commit shows how to reproduce the error shown in https://github.com/bitcoin/bitcoin/issues/30514 and how it could be fixed with a minimal change. The second commit adds an explicit sanitizer that will probably be the preferred approach. I can squash the commits but first I would like to hear conceptual feedback since https://github.com/bitcoin/bitcoin/issues/30514 suggested to remove the block height instead."
π maflcko approved a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2226246193)
left a nit. Feel free to ignore.
Otherwise this looks good on a first glance.
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2226246193)
left a nit. Feel free to ignore.
Otherwise this looks good on a first glance.
π¬ maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707958805)
If you re-touch this commit, it could make sense to remove the `__func__` printing in the log call below. It doesn't add any value and may be duplicate. I understand it is unrelated, but when touching a test-only function, may as well fix all trivial issues in one go. (See the commit I referred to in https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1703942199, which also has steps to reproduce and test in the description)
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1707958805)
If you re-touch this commit, it could make sense to remove the `__func__` printing in the log call below. It doesn't add any value and may be duplicate. I understand it is unrelated, but when touching a test-only function, may as well fix all trivial issues in one go. (See the commit I referred to in https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1703942199, which also has steps to reproduce and test in the description)
π¬ fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707962074)
We would probably have to cast somewhere else then because `nHeight` is `int`.
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707962074)
We would probably have to cast somewhere else then because `nHeight` is `int`.
β
maflcko closed a pull request: "Create wallet from seed"
(https://github.com/bitcoin/bitcoin/pull/30606)
(https://github.com/bitcoin/bitcoin/pull/30606)
π¬ fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2274401173)
> Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed
Thanks, that was not clear to me indeed.
> I haven't tried it, but https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1700350964 (the error message for the user will not be: "Corrupt metadata, block height corrupt", but instead "A forked headers-chain with more work than the chain with the snapshot base block header exists. Ple
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2274401173)
> Just to clarify, I am happy to review and ack this approach here, but I haven't acked the current state of the pull request, because it leaves bugs unfixed
Thanks, that was not clear to me indeed.
> I haven't tried it, but https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1700350964 (the error message for the user will not be: "Corrupt metadata, block height corrupt", but instead "A forked headers-chain with more work than the chain with the snapshot base block header exists. Ple
...
π theStack opened a pull request: "contrib: support reading XORed blocks in linearize-data.py script"
(https://github.com/bitcoin/bitcoin/pull/30607)
This PR is a small follow-up for #28052, adding support for the block linearization script to handle XORed blocksdir *.dat files. Note that if no xor.dat file exists, the XOR pattern is set to all-zeros, in order to still support blockdirs that have been created with versions earlier than 28.x.
Partly fixes issue #30599.
(https://github.com/bitcoin/bitcoin/pull/30607)
This PR is a small follow-up for #28052, adding support for the block linearization script to handle XORed blocksdir *.dat files. Note that if no xor.dat file exists, the XOR pattern is set to all-zeros, in order to still support blockdirs that have been created with versions earlier than 28.x.
Partly fixes issue #30599.
π¬ maflcko commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2274410714)
Dropped one commit, because a conflicting pull has already picked it up.
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2274410714)
Dropped one commit, because a conflicting pull has already picked it up.
π¬ fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2274410734)
A little recap so far: While I still donβt find the arguments *against* having the height included convincing I am currently leaning towards going with the approach in #30598 and removing it for now. My reasoning is as follows:
- While the arguments against may not be strong I conceive that it would be better if we had a clear plan for how we use the height which would give a stronger argument for keeping it in. We can re-add the height in a new version together with a clear plan of how it is
...
(https://github.com/bitcoin/bitcoin/pull/30516#issuecomment-2274410734)
A little recap so far: While I still donβt find the arguments *against* having the height included convincing I am currently leaning towards going with the approach in #30598 and removing it for now. My reasoning is as follows:
- While the arguments against may not be strong I conceive that it would be better if we had a clear plan for how we use the height which would give a stronger argument for keeping it in. We can re-add the height in a new version together with a clear plan of how it is
...
π¬ fjahr commented on pull request "Assumeutxo: Sanitize block height in metadata":
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707978449)
So I was still hunting for the conceptual consensus and didn't change this yet. I think we are coming to an end though (see my comment below). It would be great if you could indicate where you stand on this though @ismaelsadeeq . If more support for the approach here I will squash the commits and implement the suggested improvements.
(https://github.com/bitcoin/bitcoin/pull/30516#discussion_r1707978449)
So I was still hunting for the conceptual consensus and didn't change this yet. I think we are coming to an end though (see my comment below). It would be great if you could indicate where you stand on this though @ismaelsadeeq . If more support for the approach here I will squash the commits and implement the suggested improvements.
π¬ theuni commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2274440199)
Very cool. Can't wait to dig in when I have some free time.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2274440199)
Very cool. Can't wait to dig in when I have some free time.
π€ hodlinator reviewed a pull request: "Add a "tx output spender" index"
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-2219808525)
Concept ACK 40e44e96c38593210877f0b5d062e71a8dd292a5
Can see how this is useful for L2s. Is a variant of this PR known to be used in production?
I do agree with @Pantamis in https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1320276112 that having it take 3x the space of **TxIndex** seems unfortunate, would prefer it used `CDiskTxPos` for the values.
Commits could be squashed/rewritten. Seems like the latter two are responses to PR feedback rather than "telling a story".
Prev
...
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-2219808525)
Concept ACK 40e44e96c38593210877f0b5d062e71a8dd292a5
Can see how this is useful for L2s. Is a variant of this PR known to be used in production?
I do agree with @Pantamis in https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1320276112 that having it take 3x the space of **TxIndex** seems unfortunate, would prefer it used `CDiskTxPos` for the values.
Commits could be squashed/rewritten. Seems like the latter two are responses to PR feedback rather than "telling a story".
Prev
...
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1704599014)
nit: Inconsistent style, should probably have initializer list on own line in both cases.
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1704599014)
nit: Inconsistent style, should probably have initializer list on own line in both cases.
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706479714)
Suggestion: use `optional` to promote error checking or `[[nodiscard]] bool` and document non-const "out"-references by variable naming or comment.
```suggestion
std::optional<Txid> TxoSpenderIndex::FindSpender(const COutPoint& txo) const
{
uint256 tx_hash_out;
if (m_db->Read(std::pair{DB_TXOSPENDERINDEX, txo}, tx_hash_out))
return Txid::FromUint256(tx_hash_out);
return std::nullopt;
}
```
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706479714)
Suggestion: use `optional` to promote error checking or `[[nodiscard]] bool` and document non-const "out"-references by variable naming or comment.
```suggestion
std::optional<Txid> TxoSpenderIndex::FindSpender(const COutPoint& txo) const
{
uint256 tx_hash_out;
if (m_db->Read(std::pair{DB_TXOSPENDERINDEX, txo}, tx_hash_out))
return Txid::FromUint256(tx_hash_out);
return std::nullopt;
}
```
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706495582)
No longer used?
```suggestion
#include <common/args.h>
```
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706495582)
No longer used?
```suggestion
#include <common/args.h>
```
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706507458)
Could use `pair{}` for brevity here and elsewhere (https://stackoverflow.com/a/41521422).
```suggestion
batch.Write(std::pair{DB_TXOSPENDERINDEX, outpoint}, hash);
```
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706507458)
Could use `pair{}` for brevity here and elsewhere (https://stackoverflow.com/a/41521422).
```suggestion
batch.Write(std::pair{DB_TXOSPENDERINDEX, outpoint}, hash);
```
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706465407)
nit: Consider using `default` syntax to indicate that you only included this in the **.cpp** file so that `m_db` can be destructed:
```suggestion
TxoSpenderIndex::~TxoSpenderIndex() = default;
```
`TxIndex` already does this.
(Inspired by https://github.com/bitcoin/bitcoin/pull/19988#discussion_r494257310).
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706465407)
nit: Consider using `default` syntax to indicate that you only included this in the **.cpp** file so that `m_db` can be destructed:
```suggestion
TxoSpenderIndex::~TxoSpenderIndex() = default;
```
`TxIndex` already does this.
(Inspired by https://github.com/bitcoin/bitcoin/pull/19988#discussion_r494257310).
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706536603)
Suggestion: Clarify the conditional branches and state of `mempool_only` later in the code:
```suggestion
// do nothing if only mempool querying is allowed
```
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706536603)
Suggestion: Clarify the conditional branches and state of `mempool_only` later in the code:
```suggestion
// do nothing if only mempool querying is allowed
```
π¬ hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706511696)
Superfluous comment copied from **txindex.h**.
```suggestion
```
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1706511696)
Superfluous comment copied from **txindex.h**.
```suggestion
```