💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425282679)
In 671de055c8b3ff48b16b93d8389b80fead6342ac
Commit title indicate " [doc] cpfp carveout is excluded in packages "
But the diff is not a only a doc change.
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425282679)
In 671de055c8b3ff48b16b93d8389b80fead6342ac
Commit title indicate " [doc] cpfp carveout is excluded in packages "
But the diff is not a only a doc change.
💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425551039)
In 228a5ed16586ef93e7ec5c5da3ef4df0d5fc322f
```cpp
#include <alogorithm>
```
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425551039)
In 228a5ed16586ef93e7ec5c5da3ef4df0d5fc322f
```cpp
#include <alogorithm>
```
💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425583249)
Would be nice if this commit to be split into two, RBF utility functions and test in one commit and package rbf validation in another?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1425583249)
Would be nice if this commit to be split into two, RBF utility functions and test in one commit and package rbf validation in another?
💬 andrewtoth commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1872545821)
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
Ran benchmark with hyperfine:
```
hyperfine --show-output --parameter-list commit 96ec3b67a7a7f968d002e13d6fc227f69b7f07d7,d298ff8b62b2624ed390c8a2f905c888ffc956ff --setup 'git checkout {commit} && make -j$(nproc) src/bitcoind' --prepare 'sync; sudo /sbin/sysctl vm.drop_caches=3; rm -r /home/user/.bitcoin/blocks /home/user/.bitcoin/chainstate' -M 3 './src/bitcoind -dbcache=2000 -prune=2000 -printtoconsole=0 -stopatheight=800000'
```
Results w
...
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1872545821)
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
Ran benchmark with hyperfine:
```
hyperfine --show-output --parameter-list commit 96ec3b67a7a7f968d002e13d6fc227f69b7f07d7,d298ff8b62b2624ed390c8a2f905c888ffc956ff --setup 'git checkout {commit} && make -j$(nproc) src/bitcoind' --prepare 'sync; sudo /sbin/sysctl vm.drop_caches=3; rm -r /home/user/.bitcoin/blocks /home/user/.bitcoin/chainstate' -M 3 './src/bitcoind -dbcache=2000 -prune=2000 -printtoconsole=0 -stopatheight=800000'
```
Results w
...
💬 darosior commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1438670874)
Technically i think this reference can be dangling at `Shutdown()` (if there is still some wallets loaded) since we don't `reset()` the `node.wallet_loader` before resetting `node.main_signals`.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1438670874)
Technically i think this reference can be dangling at `Shutdown()` (if there is still some wallets loaded) since we don't `reset()` the `node.wallet_loader` before resetting `node.main_signals`.
💬 dimitaracev commented on pull request "wallet: move lock at the top of ReleaseWallet":
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438684168)
Correct, this was not the issue as initially thought. The removal of this is only so that we return an appropriate message if it ever comes to this instead of crashing the node. I can change it back if it matters. Not sure if it makes sense this to lead to a node crash to be honest.
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438684168)
Correct, this was not the issue as initially thought. The removal of this is only so that we return an appropriate message if it ever comes to this instead of crashing the node. I can change it back if it matters. Not sure if it makes sense this to lead to a node crash to be honest.
💬 maflcko commented on pull request "wallet: move lock at the top of ReleaseWallet":
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438714239)
I think it would be good to explain the changes, also, as mentioned previously, steps to reproduce the bug and the fix would be helpful.
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438714239)
I think it would be good to explain the changes, also, as mentioned previously, steps to reproduce the bug and the fix would be helpful.
💬 eragmus commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1872627806)
> Filtering is a very useful step. It is irrelevant to continually bring up the point that miners can still include this stuff in blocks.
Filtering by non-mining nodes is irrelevant to whether mining nodes (miners) sell their block space to those who want to buy their block space. Miners are conducting business, they have direct skin in the game, and mining is designed as a hyper competitive business.
Simultaneously, it is harmful to bitcoin for many non-mining nodes to do this, since it incen
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1872627806)
> Filtering is a very useful step. It is irrelevant to continually bring up the point that miners can still include this stuff in blocks.
Filtering by non-mining nodes is irrelevant to whether mining nodes (miners) sell their block space to those who want to buy their block space. Miners are conducting business, they have direct skin in the game, and mining is designed as a hyper competitive business.
Simultaneously, it is harmful to bitcoin for many non-mining nodes to do this, since it incen
...
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1872628019)
I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1872628019)
I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something
💬 kristapsk commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1872640782)
> I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something
Agree, makes sense. Giving read access to other users is the only use case of this functionality I know of.
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1872640782)
> I'm not sure there's a reason to ever set +w either... maybe it should just be -rpccookieaccess user/group/all or something
Agree, makes sense. Giving read access to other users is the only use case of this functionality I know of.
💬 DoctorBuzz1 commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1872925369)
> Filtering by non-mining nodes is irrelevant to whether mining nodes (miners) sell their block space to those who want to buy their block space. Miners are conducting business, they have direct skin in the game, and mining is designed as a hyper competitive business.
>
> Miners have no obligation to care what some uneconomic non-mining nodes signal about whether they subjectively don't value certain data. That's because bitcoin is neutral, and other people do subjectively value the block spac
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1872925369)
> Filtering by non-mining nodes is irrelevant to whether mining nodes (miners) sell their block space to those who want to buy their block space. Miners are conducting business, they have direct skin in the game, and mining is designed as a hyper competitive business.
>
> Miners have no obligation to care what some uneconomic non-mining nodes signal about whether they subjectively don't value certain data. That's because bitcoin is neutral, and other people do subjectively value the block spac
...
💬 furszy commented on pull request "wallet: move lock at the top of ReleaseWallet":
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438882572)
> The removal of this is only so that we return an appropriate message if it ever comes to this instead of crashing the node. I can change it back if it matters. Not sure if it makes sense this to lead to a node crash to be honest.
The current structure of the code disallows it from happening; prior to every `UnloadWallet()` call, we call `RemoveWallet()`. Which ensures that we never crash at this point (https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869817009).
So, if it crashe
...
(https://github.com/bitcoin/bitcoin/pull/29155#discussion_r1438882572)
> The removal of this is only so that we return an appropriate message if it ever comes to this instead of crashing the node. I can change it back if it matters. Not sure if it makes sense this to lead to a node crash to be honest.
The current structure of the code disallows it from happening; prior to every `UnloadWallet()` call, we call `RemoveWallet()`. Which ensures that we never crash at this point (https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1869817009).
So, if it crashe
...
💬 darosior commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1872959722)
CI is failing with
```
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::ParseScript(unsigned int&, Span<char const>&, (anonymous namespace)::ParseScriptContext, FlatSigningProvider&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)':
descriptor.cpp:(.text+0x390f): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
...
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-1872959722)
CI is failing with
```
/usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x23
libbitcoin_common.a(libbitcoin_common_a-descriptor.o): in function `(anonymous namespace)::ParseScript(unsigned int&, Span<char const>&, (anonymous namespace)::ParseScriptContext, FlatSigningProvider&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)':
descriptor.cpp:(.text+0x390f): undefined reference to `miniscript::operator"" _mst(char const*, unsigned int)'
...
💬 darosior commented on pull request "fuzz: rule-out too deep derivation paths in descriptor parsing targets":
(https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1872960181)
> Another (related) one?
>
> [clusterfuzz-testcase-miniscript_string-6556534783737856.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13472836/clusterfuzz-testcase-miniscript_string-6556534783737856.bin.not.txt)
>
> 
> Another (related) one?
>
> [clusterfuzz-testcase-miniscript_string-6556534783737856.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13472836/clusterfuzz-testcase-miniscript_string-6556534783737856.bin.not.txt)
>
> ![Screenshot from 2023-11-27 11-14-38](https://private-user-images.githubusercontent.com/6399679/285821448-eba95f4c-52fa-494c-9c92-96fdd42de46a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6Imt
...
💬 darosior commented on pull request "fuzz: rule-out too deep derivation paths in descriptor parsing targets":
(https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1872973353)
> Could you add the exception for the scriptpubkeyman harness as well?
Done by moving the introduced `HasDeepDerivPath` into `src/test/fuzz/util/descriptor.h` and also calling it in `scriptpubkeyman` target's `CreateWalletDescriptor` right before parsing the mocked descriptor string.
(https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1872973353)
> Could you add the exception for the scriptpubkeyman harness as well?
Done by moving the introduced `HasDeepDerivPath` into `src/test/fuzz/util/descriptor.h` and also calling it in `scriptpubkeyman` target's `CreateWalletDescriptor` right before parsing the mocked descriptor string.
💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1872973620)
> Another one: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64593 cc @brunoerg
I think i've fixed it in https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1872973353. Can you share the repro from oss-fuzz here so i can confirm it?
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1872973620)
> Another one: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64593 cc @brunoerg
I think i've fixed it in https://github.com/bitcoin/bitcoin/pull/28832#issuecomment-1872973353. Can you share the repro from oss-fuzz here so i can confirm it?
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1872976767)
Added a `MakeNoLogFileContext` at init to avoid the ever-increasing memory usage due to log messages.
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1872976767)
Added a `MakeNoLogFileContext` at init to avoid the ever-increasing memory usage due to log messages.
💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1872991949)
While rebasing this i found another crash in the `coins_view` target. Catching the exception on `AddCoin()` and continuing will cause `cachedCoinsUsage` to underflow:
https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/test/fuzz/coins_view.cpp#L67-L75
https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/coins.cpp#L76-L100
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1872991949)
While rebasing this i found another crash in the `coins_view` target. Catching the exception on `AddCoin()` and continuing will cause `cachedCoinsUsage` to underflow:
https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/test/fuzz/coins_view.cpp#L67-L75
https://github.com/bitcoin/bitcoin/blob/4b1196a9855dcd188a24f393aa2fa21e2d61f061/src/coins.cpp#L76-L100
💬 DoctorBuzz1 commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1873026315)
One more thing about the BRC-20 style crap.... isn't this something appropriate for isDust()??
I'm sure I don't have these variables correct (and I still think in cents, so bear with me), but using the example above....
If a (Tx == $0.23) && (TxFee ==$6.45)... then couldn't something like this be fairly usable??
If (Tx < 3TxFee) {
isDust = True};
Or some other ratio that makes sense like "If (Tx < 2TxFee)"?? I'm personally not going to send a $10 Tx for a $5 fee, honestly, b
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1873026315)
One more thing about the BRC-20 style crap.... isn't this something appropriate for isDust()??
I'm sure I don't have these variables correct (and I still think in cents, so bear with me), but using the example above....
If a (Tx == $0.23) && (TxFee ==$6.45)... then couldn't something like this be fairly usable??
If (Tx < 3TxFee) {
isDust = True};
Or some other ratio that makes sense like "If (Tx < 2TxFee)"?? I'm personally not going to send a $10 Tx for a $5 fee, honestly, b
...
💬 BitcoinMechanic commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1873178352)
> > Filtering is a very useful step. It is irrelevant to continually bring up the point that miners can still include this stuff in blocks.
>
> Filtering by non-mining nodes is irrelevant to whether mining nodes (miners) sell their block space to those who want to buy their block space. Miners are conducting business, they have direct skin in the game, and mining is designed as a hyper competitive business.
>
> Simultaneously, it is harmful to bitcoin for many non-mining nodes to do this,
...
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1873178352)
> > Filtering is a very useful step. It is irrelevant to continually bring up the point that miners can still include this stuff in blocks.
>
> Filtering by non-mining nodes is irrelevant to whether mining nodes (miners) sell their block space to those who want to buy their block space. Miners are conducting business, they have direct skin in the game, and mining is designed as a hyper competitive business.
>
> Simultaneously, it is harmful to bitcoin for many non-mining nodes to do this,
...