💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1124914676)
I think this condition is guaranteed by the other static assertions, but this expression seems incorrect. In general, `A & (B-1) == 0` doesn't mean that A is a multiple of B, does it? If A=8 and B=3, then `8 & (3-1)` is zero, but 8 isn't a multiple of 3. If A=8, B=4, then `8 & (4-1)` is also zero, so we get the correct result (8 is a multiple of 4), but it's kind of by accident. To state it differently, if `MAX_BLOCK_SIZE_BYTES` is some large power of 2 (which is guaranteed above), then in binar
...
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1124914676)
I think this condition is guaranteed by the other static assertions, but this expression seems incorrect. In general, `A & (B-1) == 0` doesn't mean that A is a multiple of B, does it? If A=8 and B=3, then `8 & (3-1)` is zero, but 8 isn't a multiple of 3. If A=8, B=4, then `8 & (4-1)` is also zero, so we get the correct result (8 is a multiple of 4), but it's kind of by accident. To state it differently, if `MAX_BLOCK_SIZE_BYTES` is some large power of 2 (which is guaranteed above), then in binar
...
💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125417231)
I don't understand the purpose of adding `sizeof(void*) * 4`; could you leave a brief comment if you get the chance? (Unless I'm just being clueless!)
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125417231)
I don't understand the purpose of adding `sizeof(void*) * 4`; could you leave a brief comment if you get the chance? (Unless I'm just being clueless!)
💬 MarcoFalke commented on pull request "util: add missing include and fix function signature":
(https://github.com/bitcoin/bitcoin/pull/27192#discussion_r1125426076)
Yeah, I was mostly thinking that it would be good to check for other missing includes while touching the file, so that it doesn't have to be touched again later if there is a missing stdlib include or so.
(https://github.com/bitcoin/bitcoin/pull/27192#discussion_r1125426076)
Yeah, I was mostly thinking that it would be good to check for other missing includes while touching the file, so that it doesn't have to be touched again later if there is a missing stdlib include or so.
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125443783)
In line 89 there is an assert that `ELEM_ALIGN_BYTES` is multiple of 2:
```cpp
static_assert((ELEM_ALIGN_BYTES & (ELEM_ALIGN_BYTES - 1)) == 0, "ELEM_ALIGN_BYTES must be a power of two");
```
So given that line 91 should make sense, `ELEM_ALIGN_BYTES - 1` becomes a bitmask and actually asserts that `MAX_BLOCK_SIZE_BYTES` is multiple of `ELEM_ALIGN_BYTES`. But right, on its own that assert wouldn't be enough and using `%` is probably a bit more clear.
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125443783)
In line 89 there is an assert that `ELEM_ALIGN_BYTES` is multiple of 2:
```cpp
static_assert((ELEM_ALIGN_BYTES & (ELEM_ALIGN_BYTES - 1)) == 0, "ELEM_ALIGN_BYTES must be a power of two");
```
So given that line 91 should make sense, `ELEM_ALIGN_BYTES - 1` becomes a bitmask and actually asserts that `MAX_BLOCK_SIZE_BYTES` is multiple of `ELEM_ALIGN_BYTES`. But right, on its own that assert wouldn't be enough and using `%` is probably a bit more clear.
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125443786)
Posting here now, I can add this as a comment later:
The value here determines the maximum bytes that the `PoolAllocator` supports. When bigger blocks are allocated, this is just forwarded to `new`.
The thing with `sizeof(void*) * 4` is, it is not enough to just support up to sizes of the `std::pair`, because the different implementations of `std::unordered_map` use more memory for each node. Most implementations wrap the std::pair into a struct that contains a single pointer, so they can
...
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1125443786)
Posting here now, I can add this as a comment later:
The value here determines the maximum bytes that the `PoolAllocator` supports. When bigger blocks are allocated, this is just forwarded to `new`.
The thing with `sizeof(void*) * 4` is, it is not enough to just support up to sizes of the `std::pair`, because the different implementations of `std::unordered_map` use more memory for each node. Most implementations wrap the std::pair into a struct that contains a single pointer, so they can
...
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1454703666)
@LarryRuane awesome that you'll hold a PR review club about this PR! I'll try to join, but can't yet guarantee that I'll have the time.
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1454703666)
@LarryRuane awesome that you'll hold a PR review club about this PR! I'll try to join, but can't yet guarantee that I'll have the time.
⚠️ mraksoll4 opened an issue: "Release 0.24.0.1 File system error."
(https://github.com/bitcoin/bitcoin/issues/27198)
Release 0.24.0.1
Executable Buildet at debian give error at windows file system.
`EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE
filesystem error: cannot remove [D:\Chains\Bitcoin22\anchors.dat]
H:\Testing\dssdds\bin\bitcoin-qt.exe in Runaway exception
2023-03-04T16:33:52Z torcontrol thread exit`
(https://github.com/bitcoin/bitcoin/issues/27198)
Release 0.24.0.1
Executable Buildet at debian give error at windows file system.
`EXCEPTION: NSt10filesystem7__cxx1116filesystem_errorE
filesystem error: cannot remove [D:\Chains\Bitcoin22\anchors.dat]
H:\Testing\dssdds\bin\bitcoin-qt.exe in Runaway exception
2023-03-04T16:33:52Z torcontrol thread exit`
💬 fjahr commented on pull request "http: Track active requests and wait for last to finish - 2nd attempt":
(https://github.com/bitcoin/bitcoin/pull/26742#discussion_r1125511981)
Thanks! I will fix this in the follow-up that will pick up #19434.
(https://github.com/bitcoin/bitcoin/pull/26742#discussion_r1125511981)
Thanks! I will fix this in the follow-up that will pick up #19434.
💬 saadbitcoin commented on pull request "http: Track active requests and wait for last to finish - 2nd attempt":
(https://github.com/bitcoin/bitcoin/pull/26742#issuecomment-1454847059)
> Running `test/functional/feature_abortnode.py` before and after change we get a significant performance gain
>
> running before changes [f3bc1a7](https://github.com/bitcoin/bitcoin/commit/f3bc1a72825fe2b51f4bc20e004cef464f05b965)
>
> ```
> test/functional/feature_abortnode.py
> 2022-12-23T04:47:19.826000Z TestFramework (INFO): Initializing test directory /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_k3_6xric
> 2022-12-23T04:47:21.229000Z TestFramework (INFO): Wait
...
(https://github.com/bitcoin/bitcoin/pull/26742#issuecomment-1454847059)
> Running `test/functional/feature_abortnode.py` before and after change we get a significant performance gain
>
> running before changes [f3bc1a7](https://github.com/bitcoin/bitcoin/commit/f3bc1a72825fe2b51f4bc20e004cef464f05b965)
>
> ```
> test/functional/feature_abortnode.py
> 2022-12-23T04:47:19.826000Z TestFramework (INFO): Initializing test directory /var/folders/9g/cvx014yx4dq5lwl_x5zvn_j80000gn/T/bitcoin_func_test_k3_6xric
> 2022-12-23T04:47:21.229000Z TestFramework (INFO): Wait
...
💬 stickies-v commented on pull request "doc: Fixup remove 'omitted...' doc for rpc getrawtransaction when verbose is 2":
(https://github.com/bitcoin/bitcoin/pull/26968#issuecomment-1454888362)
I've provided links [[1](https://github.com/bitcoin/bitcoin/pull/26968#discussion_r1087741997), [2](https://github.com/bitcoin/bitcoin/pull/26968#discussion_r1087746520)] in my previous comments to reference the code from which I've built my understanding about which fields are optional and which ones aren't. Are we disagreeing about behaviour? My understanding in short is: "vin" is always present for verbosity>=1, whereas "fee" and "prevout" are omitted if no block data.
So to me it seems l
...
(https://github.com/bitcoin/bitcoin/pull/26968#issuecomment-1454888362)
I've provided links [[1](https://github.com/bitcoin/bitcoin/pull/26968#discussion_r1087741997), [2](https://github.com/bitcoin/bitcoin/pull/26968#discussion_r1087746520)] in my previous comments to reference the code from which I've built my understanding about which fields are optional and which ones aren't. Are we disagreeing about behaviour? My understanding in short is: "vin" is always present for verbosity>=1, whereas "fee" and "prevout" are omitted if no block data.
So to me it seems l
...
💬 TheCharlatan commented on pull request "blockstorage: add an assert to avoid running oom with `-fastprune`":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1125565424)
Instead of asserting, how about setting the block file size dynamically in case the block to be written exceeds the limit? Something like:
```
- const unsigned int max_blockfile_size{gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64kb */ : MAX_BLOCKFILE_SIZE};
+ unsigned int max_blockfile_size = MAX_BLOCKFILE_SIZE;
+ if (gArgs.GetBoolArg("-fastprune", false)) {
+ max_blockfile_size = 0x10000; // 64kiB
+ if (nAddSize >= max_blockfile_size) {
+
...
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1125565424)
Instead of asserting, how about setting the block file size dynamically in case the block to be written exceeds the limit? Something like:
```
- const unsigned int max_blockfile_size{gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64kb */ : MAX_BLOCKFILE_SIZE};
+ unsigned int max_blockfile_size = MAX_BLOCKFILE_SIZE;
+ if (gArgs.GetBoolArg("-fastprune", false)) {
+ max_blockfile_size = 0x10000; // 64kiB
+ if (nAddSize >= max_blockfile_size) {
+
...
📝 ishaanam opened a pull request: "test: fix race condition in encrypted wallet rescan tests"
(https://github.com/bitcoin/bitcoin/pull/27199)
This fixes https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123340738
(https://github.com/bitcoin/bitcoin/pull/27199)
This fixes https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123340738
💬 ishaanam commented on pull request "wallet: ensure the wallet is unlocked when needed for rescanning":
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1125568137)
Ok, I think I understand what you mean now. I have implemented this using multi-threading in #27199. However, for me this no longer fails without the fix.
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1125568137)
Ok, I think I understand what you mean now. I have implemented this using multi-threading in #27199. However, for me this no longer fails without the fix.
💬 kiminuo commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#discussion_r1125568212)
@0xB10C I modified
```cpp
for (const CTxMemPoolEntry& e : pool.mapTx) {
const CAmount fee{e.GetFee()};
const uint32_t size{uint32_t(e.GetTxSize())};
const CAmount fee_rate{CFeeRate{fee, size}.GetFee(1)};
// Distribute fee rates
for (size_t i = floors.size(); i-- > 0;) {
if (fee_rate >= floors[i]) {
sizes[i] += size;
++count[i];
fees[i] += fee;
break;
}
}
}
```
to
```cpp
std::vecto
...
(https://github.com/bitcoin/bitcoin/pull/21422#discussion_r1125568212)
@0xB10C I modified
```cpp
for (const CTxMemPoolEntry& e : pool.mapTx) {
const CAmount fee{e.GetFee()};
const uint32_t size{uint32_t(e.GetTxSize())};
const CAmount fee_rate{CFeeRate{fee, size}.GetFee(1)};
// Distribute fee rates
for (size_t i = floors.size(); i-- > 0;) {
if (fee_rate >= floors[i]) {
sizes[i] += size;
++count[i];
fees[i] += fee;
break;
}
}
}
```
to
```cpp
std::vecto
...
💬 LarryRuane commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1125575680)
Thanks, @MarcoFalke, good suggestion, [this branch](https://github.com/LarryRuane/bitcoin/commits/pr26957-1) changes that commit to not touch production code ([individual commit](https://github.com/LarryRuane/bitcoin/commit/a1cd3cc17b9fdce1d60d662c447f845a12063d81)) @jonatack, feel free to cherry-pick.
(https://github.com/bitcoin/bitcoin/pull/26957#discussion_r1125575680)
Thanks, @MarcoFalke, good suggestion, [this branch](https://github.com/LarryRuane/bitcoin/commits/pr26957-1) changes that commit to not touch production code ([individual commit](https://github.com/LarryRuane/bitcoin/commit/a1cd3cc17b9fdce1d60d662c447f845a12063d81)) @jonatack, feel free to cherry-pick.
✅ mraksoll4 closed an issue: "Release 0.24.0.1 File system error."
(https://github.com/bitcoin/bitcoin/issues/27198)
(https://github.com/bitcoin/bitcoin/issues/27198)
📝 theStack opened a pull request: "test: psbt: check non-witness UTXO removal for segwit v1 input"
(https://github.com/bitcoin/bitcoin/pull/27200)
This PR adds missing test coverage for dropping non-witness UTXOs from PSBTs for segwit v1+ inputs (see commit 103c6fd2791f7e73eeab7f3900fbedd5b550211d). The formerly [disabled](46004790588c24174a0bec49b540d158ce163ffd) method `test_utxo_conversion` is re-enabled and adapted to spend a Taproot (`bech32m`) instead of a wrapped SegWit (`p2sh-segwit`) output. Note that in contrast to the original test, we have to add the non-witness UTXO manually here using the test framework's PSBT module, since t
...
(https://github.com/bitcoin/bitcoin/pull/27200)
This PR adds missing test coverage for dropping non-witness UTXOs from PSBTs for segwit v1+ inputs (see commit 103c6fd2791f7e73eeab7f3900fbedd5b550211d). The formerly [disabled](46004790588c24174a0bec49b540d158ce163ffd) method `test_utxo_conversion` is re-enabled and adapted to spend a Taproot (`bech32m`) instead of a wrapped SegWit (`p2sh-segwit`) output. Note that in contrast to the original test, we have to add the non-witness UTXO manually here using the test framework's PSBT module, since t
...
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1455001809)
@ryanofsky
> I think there is a potential problem with the implementation though because it isn't recursively marking conflicted transactions as unconfirmed, only marking the top level transactions without recursing.
Yes, I had forgotten about this, thanks for pointing it out!
> I think a good approach could be to steal from the previous implementation of this idea in commits https://github.com/bitcoin/bitcoin/commit/b8b4592d985fa36dd0dddfd0e2bc6daf8df6e79c and https://github.com/bitcoin/
...
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1455001809)
@ryanofsky
> I think there is a potential problem with the implementation though because it isn't recursively marking conflicted transactions as unconfirmed, only marking the top level transactions without recursing.
Yes, I had forgotten about this, thanks for pointing it out!
> I think a good approach could be to steal from the previous implementation of this idea in commits https://github.com/bitcoin/bitcoin/commit/b8b4592d985fa36dd0dddfd0e2bc6daf8df6e79c and https://github.com/bitcoin/
...
💬 fanquake commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1455040012)
Is this a draft because you haven't tested under Valgrind yet? Could add to the PR description what needs doing.
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1455040012)
Is this a draft because you haven't tested under Valgrind yet? Could add to the PR description what needs doing.
⚠️ da2ce7 opened an issue: "Full Support for Spending Untrusted Unconfirmed Outputs"
(https://github.com/bitcoin/bitcoin/issues/27202)
Spending untrusted unconfirmed outputs should be supported and robust.
Key feature:
1. Calculation of the appropriate fee based upon the transaction package.
2. When a dependent transaction is modified, (for example RBF increase), the dependent transaction should be updated appropriately. Users can optionally automate this process, and keep the appropriate signing keys online until the transaction package confirms (as this involves decreasing the transaction fee as the dependent transacti
...
(https://github.com/bitcoin/bitcoin/issues/27202)
Spending untrusted unconfirmed outputs should be supported and robust.
Key feature:
1. Calculation of the appropriate fee based upon the transaction package.
2. When a dependent transaction is modified, (for example RBF increase), the dependent transaction should be updated appropriately. Users can optionally automate this process, and keep the appropriate signing keys online until the transaction package confirms (as this involves decreasing the transaction fee as the dependent transacti
...