💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1386191801)
in 95153f0152c057101e894e2ca73aee78be267b33
nit: Should we just add serialization methods to `CExtPubKey`? Instead of explicitly calling `Encode()`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1386191801)
in 95153f0152c057101e894e2ca73aee78be267b33
nit: Should we just add serialization methods to `CExtPubKey`? Instead of explicitly calling `Encode()`
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390404008)
in 96365a0b182858d686e695a153af232c7b3740c9
check return value
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390404008)
in 96365a0b182858d686e695a153af232c7b3740c9
check return value
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390408127)
in e693bd02610df20f73c008b03c739da5d69fcf76
nit: If the key is not valid - it should be an error and it should be detected in `LoadActiveHDKey()`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390408127)
in e693bd02610df20f73c008b03c739da5d69fcf76
nit: If the key is not valid - it should be an error and it should be detected in `LoadActiveHDKey()`
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390409054)
in e693bd02610df20f73c008b03c739da5d69fcf76
nit: inconsistent naming.
Propose to rename to `GetActiveHDKey()` analogous to `LoadActiveHDKey()` and `SetActiveHDKey()`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390409054)
in e693bd02610df20f73c008b03c739da5d69fcf76
nit: inconsistent naming.
Propose to rename to `GetActiveHDKey()` analogous to `LoadActiveHDKey()` and `SetActiveHDKey()`
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390410691)
in 9900207360a2852041ceed260c3430e92ff860c5
The comment is a bit outdated it seems, since the method can also return xprv.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390410691)
in 9900207360a2852041ceed260c3430e92ff860c5
The comment is a bit outdated it seems, since the method can also return xprv.
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390410589)
in 9900207360a2852041ceed260c3430e92ff860c5
The RPC can return xprv as well as xpub. We should find a better name as it becomes our API and users will depend on it.
How about `gethdkey`?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390410589)
in 9900207360a2852041ceed260c3430e92ff860c5
The RPC can return xprv as well as xpub. We should find a better name as it becomes our API and users will depend on it.
How about `gethdkey`?
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390411824)
in 9900207360a2852041ceed260c3430e92ff860c5
nit: "Could not find the xprv for active **HD** key"
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390411824)
in 9900207360a2852041ceed260c3430e92ff860c5
nit: "Could not find the xprv for active **HD** key"
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390418490)
Should we add some documentation about this quirk?
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1390418490)
Should we add some documentation about this quirk?
👍 hebasto approved a pull request: "doc: update docs for `CHECK_ATOMIC` macro"
(https://github.com/bitcoin/bitcoin/pull/28777#pullrequestreview-1726270348)
ACK ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7.
(https://github.com/bitcoin/bitcoin/pull/28777#pullrequestreview-1726270348)
ACK ebc7063c80135dd6f3e7b9418e8f4bf217bd8db7.
💬 hebasto commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1807197107)
Testing v26.0rc2 on an `armv7l` device with 2 GB of RAM.
It fails to catch up with OOM when the cache size reaches appr. 125 MiB.
There is no such an issue when running v25.1.
The `bitcoin.conf` file contains:
```
dbcache=100 # default=450
par=4 # default=0; nproc returns 8
disablewallet=1
mempoolfullrbf=1
```
All binaries are downloaded from https://bitcoincore.org/bin/.
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1807197107)
Testing v26.0rc2 on an `armv7l` device with 2 GB of RAM.
It fails to catch up with OOM when the cache size reaches appr. 125 MiB.
There is no such an issue when running v25.1.
The `bitcoin.conf` file contains:
```
dbcache=100 # default=450
par=4 # default=0; nproc returns 8
disablewallet=1
mempoolfullrbf=1
```
All binaries are downloaded from https://bitcoincore.org/bin/.
💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1390462790)
I think can be `>` only. Generally sounds maximum in net_processing is understood as strictly superior, e.g `MAX_INV_SZ` usage.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1390462790)
I think can be `>` only. Generally sounds maximum in net_processing is understood as strictly superior, e.g `MAX_INV_SZ` usage.
💬 ariard commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1390462533)
I think a test can be added to exercise `AddToSet` and the check of the boundary `MAX_SET_SIZE` value.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1390462533)
I think a test can be added to exercise `AddToSet` and the check of the boundary `MAX_SET_SIZE` value.
🤔 theStack reviewed a pull request: "Improve peformance of CTransaction::HasWitness (28107 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/28766#pullrequestreview-1726353809)
The code looks correct.
I'm still unsure on how to assess whether the extra memory needed is okay or not, other than "gut feeling". I would have expected that the increased size of `CTransaction` (on my system, it's 128 bytes on the PR vs. 120 bytes on master, a growth of ~6.66%) has a tiny impact on the mempool usage accounting as well, but it seems that it doesn't, as `RecursiveDynamicUsage` only cares about a tx's inputs and outputs fields:
https://github.com/bitcoin/bitcoin/blob/82437627
...
(https://github.com/bitcoin/bitcoin/pull/28766#pullrequestreview-1726353809)
The code looks correct.
I'm still unsure on how to assess whether the extra memory needed is okay or not, other than "gut feeling". I would have expected that the increased size of `CTransaction` (on my system, it's 128 bytes on the PR vs. 120 bytes on master, a growth of ~6.66%) has a tiny impact on the mempool usage accounting as well, but it seems that it doesn't, as `RecursiveDynamicUsage` only cares about a tx's inputs and outputs fields:
https://github.com/bitcoin/bitcoin/blob/82437627
...
💬 sipa commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807273201)
@theStack The dynamic memory isn't the only thing that matters, but it's the only thing you don't already account for at another level.
If you have a single transaction, and want its memory usage, use `sizeof(transaction)` + DynamicMemoryUsage(transaction)`. It'd be convenient to have the transaction sizeof added to that memory usage. However, things would go wrong if you use a collection then.
Say you have a vector of transactions, and want its memory usage. You'd use `sizeof(vector)` + dynam
...
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807273201)
@theStack The dynamic memory isn't the only thing that matters, but it's the only thing you don't already account for at another level.
If you have a single transaction, and want its memory usage, use `sizeof(transaction)` + DynamicMemoryUsage(transaction)`. It'd be convenient to have the transaction sizeof added to that memory usage. However, things would go wrong if you use a collection then.
Say you have a vector of transactions, and want its memory usage. You'd use `sizeof(vector)` + dynam
...
💬 theStack commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807285127)
@sipa: Thanks for clarifying, that makes sense and I see now why it would be a bad idea to add the static part directly in the `DynamicMemoryUsage` functions. For the specific case of mempool usage accounting, one could add `sizeof(CTransaction)` at the call-site though? E.g.
```diff
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 1f175a5ccf..95362fe2d9 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -108,7 +108,7 @@ public:
...
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807285127)
@sipa: Thanks for clarifying, that makes sense and I see now why it would be a bad idea to add the static part directly in the `DynamicMemoryUsage` functions. For the specific case of mempool usage accounting, one could add `sizeof(CTransaction)` at the call-site though? E.g.
```diff
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 1f175a5ccf..95362fe2d9 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -108,7 +108,7 @@ public:
...
💬 sipa commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807326135)
@theStack Yeah, I think that'd be closer to what one would expect such a number to mean.
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807326135)
@theStack Yeah, I think that'd be closer to what one would expect such a number to mean.
💬 sipa commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807345702)
@theStack (deleted my previous comment, I had missed a lot).
`RecursiveDynamicUsage` for `CTransactionRef` (which is a `std::shared_ptr<CTransaction>`) should already account for both the memory allocated in the shared pointer as well as the memory allocated by the transaction. See `src/core_memusage.h`:
```c++
template<typename X>
static inline size_t RecursiveDynamicUsage(const std::shared_ptr<X>& p) {
return p ? memusage::DynamicUsage(p) + RecursiveDynamicUsage(*p) : 0;
}
```
...
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807345702)
@theStack (deleted my previous comment, I had missed a lot).
`RecursiveDynamicUsage` for `CTransactionRef` (which is a `std::shared_ptr<CTransaction>`) should already account for both the memory allocated in the shared pointer as well as the memory allocated by the transaction. See `src/core_memusage.h`:
```c++
template<typename X>
static inline size_t RecursiveDynamicUsage(const std::shared_ptr<X>& p) {
return p ? memusage::DynamicUsage(p) + RecursiveDynamicUsage(*p) : 0;
}
```
...
💬 theStack commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807405302)
@sipa: Ah, very interesting, have to admit I completely overlooked `RecursiveDynamicUsage(const std::shared_ptr<X>& p)`.
> Is it possible that you just don't see the added field cause changes because either it's fitting in earlier padding space, or because the rounding up effect of `malloc()` hides the difference?
Yes, it was the latter. The added field did lead to a size increase of `CTransaction` (from 120 to 120 bytes), but `MallocUsage` indeed yields the same value for both of these [s
...
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1807405302)
@sipa: Ah, very interesting, have to admit I completely overlooked `RecursiveDynamicUsage(const std::shared_ptr<X>& p)`.
> Is it possible that you just don't see the added field cause changes because either it's fitting in earlier padding space, or because the rounding up effect of `malloc()` hides the difference?
Yes, it was the latter. The added field did lead to a size increase of `CTransaction` (from 120 to 120 bytes), but `MallocUsage` indeed yields the same value for both of these [s
...
💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1390623656)
Thanks!
I think you should actually use `net` by making all callers from `p2p.py` pass it to `EncryptedP2PState`, the default value of 'regtest' should be dropped in my opinion. You can verify that it works when `p2p_dos_header_tree.py --transport` doesn't fail anymore.
Could also change all existing imports for MAGIC_BYTES (there are a few in other tests) to avoid indirect imports.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1390623656)
Thanks!
I think you should actually use `net` by making all callers from `p2p.py` pass it to `EncryptedP2PState`, the default value of 'regtest' should be dropped in my opinion. You can verify that it works when `p2p_dos_header_tree.py --transport` doesn't fail anymore.
Could also change all existing imports for MAGIC_BYTES (there are a few in other tests) to avoid indirect imports.
💬 Sjors commented on pull request "script, assumeutxo: Enhance validations in utxo_snapshot.sh":
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1807534141)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28852#issuecomment-1807534141)
Concept ACK