💬 furszy commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1204852855)
tiny nit: could wrap the functions around a "namespace wallet {}" instead.
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1204852855)
tiny nit: could wrap the functions around a "namespace wallet {}" instead.
💬 theStack commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1204878968)
Should probably also mention here that the inputs are popped from the stack? E.g. something like
`pop two top stack items and push 1 if they are equal, 0 otherwise`
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1204878968)
Should probably also mention here that the inputs are popped from the stack? E.g. something like
`pop two top stack items and push 1 if they are equal, 0 otherwise`
💬 theStack commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1204876854)
```suggestion
OP_IFDUP = 0x73, // if top stack value is true, duplicate it
```
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1204876854)
```suggestion
OP_IFDUP = 0x73, // if top stack value is true, duplicate it
```
💬 theuni commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204902245)
Sure. I only asked because it looked like it was possible in this case. But this can be done after-the-fact if we decide that's what we want.
Longer-term, we'd want to detach from our internal structures to avoid changing the abi every time we change something internally. So I don't imagine `CBlockIndex` ever being part of a public api. Maybe a trivial wrapper or something, but not CBlockIndex itself. (It's also worth keeping that in mind at this stage because that external-to-internal-compon
...
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204902245)
Sure. I only asked because it looked like it was possible in this case. But this can be done after-the-fact if we decide that's what we want.
Longer-term, we'd want to detach from our internal structures to avoid changing the abi every time we change something internally. So I don't imagine `CBlockIndex` ever being part of a public api. Maybe a trivial wrapper or something, but not CBlockIndex itself. (It's also worth keeping that in mind at this stage because that external-to-internal-compon
...
📝 russeree opened a pull request: "Use 'byte'/'bytes' for bech32(m) validation error message - 27727 follow up."
(https://github.com/bitcoin/bitcoin/pull/27747)
This PR rectifies a linguistic inconsistency found in merged PR [27727](https://github.com/bitcoin/bitcoin/pull/27727). It addresses the improper usage of the term 'byte' in error reports. As it stands, PR [27727](https://github.com/bitcoin/bitcoin/pull/27727) exclusively utilizes 'byte' in error messages, regardless of the context, as demonstrated below:
Currently: ```Invalid Bech32 v0 address program size (16 byte), per BIP141```
This modification enhances the accuracy of error reporting
...
(https://github.com/bitcoin/bitcoin/pull/27747)
This PR rectifies a linguistic inconsistency found in merged PR [27727](https://github.com/bitcoin/bitcoin/pull/27727). It addresses the improper usage of the term 'byte' in error reports. As it stands, PR [27727](https://github.com/bitcoin/bitcoin/pull/27727) exclusively utilizes 'byte' in error messages, regardless of the context, as demonstrated below:
Currently: ```Invalid Bech32 v0 address program size (16 byte), per BIP141```
This modification enhances the accuracy of error reporting
...
💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1562164731)
Thanks! I've fixed it.
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1562164731)
Thanks! I've fixed it.
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204928358)
not anymore. Re-worked the first commit so it only touches the `wallet_balance.cpp` bench file and nothing else.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204928358)
not anymore. Re-worked the first commit so it only touches the `wallet_balance.cpp` bench file and nothing else.
💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204932281)
Ended up pushing a simpler approach. Focused on correcting `wallet_balance.cpp` only.
In the future, we could generate a fake chain instead of creating a "real" one. Similar to how it's done
in the `wallet_create_tx.cpp` benchmark.
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1204932281)
Ended up pushing a simpler approach. Focused on correcting `wallet_balance.cpp` only.
In the future, we could generate a fake chain instead of creating a "real" one. Similar to how it's done
in the `wallet_create_tx.cpp` benchmark.
📝 LarryRuane opened a pull request: "bugfix: account for system-allocated memory in pool resource"
(https://github.com/bitcoin/bitcoin/pull/27748)
Follow-on to PR #25325, "Add pool based memory resource"
The `DynamicUsage()` function for the version of `unordered_map` that uses the `PoolAllocator` returns a close approximation of the amount of physical memory used by the map. (This is the map used for the dbcache.) It accounts for several of the allocator's internal data structures, such as the memory chunks, the freelist, and the unordered_map's bucket list. But it doesn't account for memory that had to be obtained from the system allo
...
(https://github.com/bitcoin/bitcoin/pull/27748)
Follow-on to PR #25325, "Add pool based memory resource"
The `DynamicUsage()` function for the version of `unordered_map` that uses the `PoolAllocator` returns a close approximation of the amount of physical memory used by the map. (This is the map used for the dbcache.) It accounts for several of the allocator's internal data structures, such as the memory chunks, the freelist, and the unordered_map's bucket list. But it doesn't account for memory that had to be obtained from the system allo
...
💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204957663)
This test may be somewhat fragile; it depends on `MallocUsage(8) == 32`. Maybe the test condition here should be `8 <= resource.SystemBytes()`, but I think that doesn't test enough.
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204957663)
This test may be somewhat fragile; it depends on `MallocUsage(8) == 32`. Maybe the test condition here should be `8 <= resource.SystemBytes()`, but I think that doesn't test enough.
💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204956602)
Copied this from https://github.com/bitcoin/bitcoin/blob/a13f3746dccd9c4ec16d6bfe9b33ebd26e3238e1/src/memusage.h#L50 because including that file would create a circular dependency. Is there a better way to do this?
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204956602)
Copied this from https://github.com/bitcoin/bitcoin/blob/a13f3746dccd9c4ec16d6bfe9b33ebd26e3238e1/src/memusage.h#L50 because including that file would create a circular dependency. Is there a better way to do this?
💬 denavila commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1562231512)
> You can leave this open and rebase it onto the main repo PR, just mention it in the description.
All right, I'll start working on this refactor.
Btw, do we have some docs on how to setup git locally so I can have dependent changes across the "bitcoin" and the "gui" repos? So far I was moving changes via patches but that doesn't seem too convenient.
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1562231512)
> You can leave this open and rebase it onto the main repo PR, just mention it in the description.
All right, I'll start working on this refactor.
Btw, do we have some docs on how to setup git locally so I can have dependent changes across the "bitcoin" and the "gui" repos? So far I was moving changes via patches but that doesn't seem too convenient.
💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562239541)
How much of a difference does this make? I ran IBD on Ubuntu using the default `dbcache` (450) and `maxmempool` (300), which gives a total cache size (during IBD) of about 750 MB, and this (missing) system allocation amount was 97 MB (96941536 bytes).
With this amount of allocation omitted from the memory usage estimate (that is, current master), nodes will use 97 MB more memory than expected, or about 13% more than configured, a pretty significant error. This regression could cause surprises
...
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562239541)
How much of a difference does this make? I ran IBD on Ubuntu using the default `dbcache` (450) and `maxmempool` (300), which gives a total cache size (during IBD) of about 750 MB, and this (missing) system allocation amount was 97 MB (96941536 bytes).
With this amount of allocation omitted from the memory usage estimate (that is, current master), nodes will use 97 MB more memory than expected, or about 13% more than configured, a pretty significant error. This regression could cause surprises
...
💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204976258)
CI failed here (on 32-bit), so I force-pushed to make the test more precise.
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204976258)
CI failed here (on 32-bit), so I force-pushed to make the test more precise.
💬 martinus commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204991822)
I don't think that calculation is now more correct, in fact this now counts the the index array twice; once in `usage_system` and once in the estimated `usage_buckets`.
The most correct way is probably to just let the pool do all the malloc & free accounting, and don't do any other estimation. Then the `DynamicUsage` method boils down to something like `return m.get_allocator().resource().memory_usage();`. That would give more theoretically correct numbers, but in practice I doubt that this i
...
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1204991822)
I don't think that calculation is now more correct, in fact this now counts the the index array twice; once in `usage_system` and once in the estimated `usage_buckets`.
The most correct way is probably to just let the pool do all the malloc & free accounting, and don't do any other estimation. Then the `DynamicUsage` method boils down to something like `return m.get_allocator().resource().memory_usage();`. That would give more theoretically correct numbers, but in practice I doubt that this i
...
💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205009503)
I see, yes, it's counting the buckets twice, thanks. I agree that either way gives the same answer; the way I'm suggesting here would isolate this pool code a little more from the details of `std::unordered_map` (i.e., the existence of the `m.bucket_count()` method), which would be a little more general, but I think the way it is now is fine. I guess if we use this pool resource for a different kind of container (for example, an ordered map), then this PR's approach would be better since we woul
...
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205009503)
I see, yes, it's counting the buckets twice, thanks. I agree that either way gives the same answer; the way I'm suggesting here would isolate this pool code a little more from the details of `std::unordered_map` (i.e., the existence of the `m.bucket_count()` method), which would be a little more general, but I think the way it is now is fine. I guess if we use this pool resource for a different kind of container (for example, an ordered map), then this PR's approach would be better since we woul
...
💬 LarryRuane commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562291221)
Closing at least for now, see the [comment](https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205009503) above.
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562291221)
Closing at least for now, see the [comment](https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205009503) above.
✅ LarryRuane closed a pull request: "bugfix: account for system-allocated memory in pool resource"
(https://github.com/bitcoin/bitcoin/pull/27748)
(https://github.com/bitcoin/bitcoin/pull/27748)
💬 martinus commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562304483)
Also related: #26614
(https://github.com/bitcoin/bitcoin/pull/27748#issuecomment-1562304483)
Also related: #26614
💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1205026761)
> And in the wallet, we use the clock time to set the descriptors/keys creation time.
Ok, thanks for the background. However, I think the wallet uses mock time (NodeClock) for the times, so a much simpler fix would be just a single line patch to set the mock time to the genesis block time in the wallet bench that needs it?
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1205026761)
> And in the wallet, we use the clock time to set the descriptors/keys creation time.
Ok, thanks for the background. However, I think the wallet uses mock time (NodeClock) for the times, so a much simpler fix would be just a single line patch to set the mock time to the genesis block time in the wallet bench that needs it?