💬 hebasto commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866158665)
Concept ACK on moving the `script/bitcoinconsensus.cpp` out from the internal `libbitcoin_consensus.a` library.
@theuni
> I think another way of looking at this is that `script/bitcoinconsensus.cpp` is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we'd actually want to build that convenience lib.
That is very similar to what I suggested in https://github.com/bitcoin/bitcoin/pull/2499
...
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1866158665)
Concept ACK on moving the `script/bitcoinconsensus.cpp` out from the internal `libbitcoin_consensus.a` library.
@theuni
> I think another way of looking at this is that `script/bitcoinconsensus.cpp` is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we'd actually want to build that convenience lib.
That is very similar to what I suggested in https://github.com/bitcoin/bitcoin/pull/2499
...
💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1866160684)
> Yes, that's what the compiler error says, but that function doesn't seem to be in that namespace. Its other callers don't qualify it, and if I do qualify it, the build fails for me.
Seems that the function was moved to the `util` namespace on #28075.
Try cleaning up your project and rebasing it on master. (git clean -fdx && git pull --rebase origin master && make).
Will need to do some modifications here. `LockDirectory()` does not return a boolean anymore.
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1866160684)
> Yes, that's what the compiler error says, but that function doesn't seem to be in that namespace. Its other callers don't qualify it, and if I do qualify it, the build fails for me.
Seems that the function was moved to the `util` namespace on #28075.
Try cleaning up your project and rebasing it on master. (git clean -fdx && git pull --rebase origin master && make).
Will need to do some modifications here. `LockDirectory()` does not return a boolean anymore.
📝 brunoerg opened a pull request: "wallet, rpc: add BIP44 `account` in `createwallet` "
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.
(https://github.com/bitcoin/bitcoin/pull/29129)
This PR adds an `account` parameter in `createwallet` RPC. It's the
BIP44 account that will be used in `SetupDescriptorScriptPubKeyMans`
to fetch the descriptors from the external signer.
👋 fanquake's pull request is ready for review: "build: switch to using LLVM 17.x for macOS builds"
(https://github.com/bitcoin/bitcoin/pull/28880)
(https://github.com/bitcoin/bitcoin/pull/28880)
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866292269)
Guix build (x86_64 & aarch64):
```bash
65981453fcb83338ed76435dc8fed06e9903441731b461e4f2c8a04ad102043c guix-build-b335710782c2/output/aarch64-linux-gnu/SHA256SUMS.part
3127d18bc55a7a207eb7e1252feaf24f767d5983e5d064323671139659c4f3d7 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu-debug.tar.gz
0a67737c010e742249f07dd3ca3ae289df02edf2dedd8d6a5a07b291921bab47 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu.tar.g
...
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866292269)
Guix build (x86_64 & aarch64):
```bash
65981453fcb83338ed76435dc8fed06e9903441731b461e4f2c8a04ad102043c guix-build-b335710782c2/output/aarch64-linux-gnu/SHA256SUMS.part
3127d18bc55a7a207eb7e1252feaf24f767d5983e5d064323671139659c4f3d7 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu-debug.tar.gz
0a67737c010e742249f07dd3ca3ae289df02edf2dedd8d6a5a07b291921bab47 guix-build-b335710782c2/output/aarch64-linux-gnu/bitcoin-b335710782c2-aarch64-linux-gnu.tar.g
...
💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866295932)
This is now reproducible, and reviewable.
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1866295932)
This is now reproducible, and reviewable.
👍 BrandonOdiwuor approved a pull request: "wallettool: Always be able to dump a wallet's database"
(https://github.com/bitcoin/bitcoin/pull/29117#pullrequestreview-1793028431)
Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995
(https://github.com/bitcoin/bitcoin/pull/29117#pullrequestreview-1793028431)
Code Review ACK d83bea42d1f0ffb0899a6de3556c489543468995
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432797027)
Rule 5 is not accurately checked here.
IMO its better to remove it completely and depend on `ApplyV3Rules` for an accurate child size limit check.
The method might even be removed completely in the future cc #28345
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432797027)
Rule 5 is not accurately checked here.
IMO its better to remove it completely and depend on `ApplyV3Rules` for an accurate child size limit check.
The method might even be removed completely in the future cc #28345
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432787931)
iwyu
```suggestion
#include <set>
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432787931)
iwyu
```suggestion
#include <set>
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432801208)
We can just return early if all the transactions are not v3
```suggestion
const auto any_v3 = std::any_of(package.begin(), package.end(), [](const auto& tx){ return tx->nVersion == 3;});
if (!any_v3) { return v3_ancestor_sets;}
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432801208)
We can just return early if all the transactions are not v3
```suggestion
const auto any_v3 = std::any_of(package.begin(), package.end(), [](const auto& tx){ return tx->nVersion == 3;});
if (!any_v3) { return v3_ancestor_sets;}
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432807598)
Nit: I find `tx_iter` more readable than `package_iter`, since its a transaction iterator not package iterator
```suggestion
for (auto tx_iter = package.rbegin(); tx_iter != package.rend(); ++tx_iter) {
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432807598)
Nit: I find `tx_iter` more readable than `package_iter`, since its a transaction iterator not package iterator
```suggestion
for (auto tx_iter = package.rbegin(); tx_iter != package.rend(); ++tx_iter) {
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432824698)
We can just skip if the ancestor has the transaction in it's descendant set?
```suggestion
if (ancestors_descendant_set.count((*tx_iter)->GetHash()) == 0 ) {
ancestors_descendant_set.insert(my_descendant_set.cbegin(), my_descendant_set.cend());
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432824698)
We can just skip if the ancestor has the transaction in it's descendant set?
```suggestion
if (ancestors_descendant_set.count((*tx_iter)->GetHash()) == 0 ) {
ancestors_descendant_set.insert(my_descendant_set.cbegin(), my_descendant_set.cend());
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432811937)
Nit: IMO its more readable to define an intermediate txid variable here also just like its done in the for loop above
```suggestion
const Txid& my_txid{(*package_iter)->GetHash()};
if ((*package_iter)->nVersion == 3) {
```
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1432811937)
Nit: IMO its more readable to define an intermediate txid variable here also just like its done in the for loop above
```suggestion
const Txid& my_txid{(*package_iter)->GetHash()};
if ((*package_iter)->nVersion == 3) {
```
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434138603)
why increment `num_prechecks_passed` here ?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434138603)
why increment `num_prechecks_passed` here ?
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434137263)
After restarting here, we are restarting at the beginning of the test again with extra args
Dev notes says
> [Avoid stop-starting the nodes multiple times during the test if possible. A stop-start takes several seconds, so doing it several times blows up the runtime of the test.](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)
The wrapper can accept the extra args as argument and restart at the beginning only ?
<details>
```python
def cleanup(
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434137263)
After restarting here, we are restarting at the beginning of the test again with extra args
Dev notes says
> [Avoid stop-starting the nodes multiple times during the test if possible. A stop-start takes several seconds, so doing it several times blows up the runtime of the test.](https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)
The wrapper can accept the extra args as argument and restart at the beginning only ?
<details>
```python
def cleanup(
...
💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434122383)
`CheckMempoolV3Invariants` docstring says we would verify
>any non-v3 tx must only have non-v3 ancestors
We are only getting parents and checking the first parent?
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434122383)
`CheckMempoolV3Invariants` docstring says we would verify
>any non-v3 tx must only have non-v3 ancestors
We are only getting parents and checking the first parent?
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434150244)
I disagree, I think we should exit as early as possible when something is too big and avoid needing to do expensive things like load a bunch of UTXOs from disk. The next line says "Important: this function is insufficient to enforce" etc.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434150244)
I disagree, I think we should exit as early as possible when something is too big and avoid needing to do expensive things like load a bunch of UTXOs from disk. The next line says "Important: this function is insufficient to enforce" etc.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434152018)
This function is documented as "Should not be called for non-v3 packages."
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434152018)
This function is documented as "Should not be called for non-v3 packages."
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434153256)
There's only 1 parent allowed, so this code takes a shortcut of only looking at the first one. Maybe it's better to edit the function params so callers can only pass 1 entry in.
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434153256)
There's only 1 parent allowed, so this code takes a shortcut of only looking at the first one. Maybe it's better to edit the function params so callers can only pass 1 entry in.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434154836)
Ah you're right, fixing
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1434154836)
Ah you're right, fixing