Bitcoin Core Github
44 subscribers
120K links
Download Telegram
πŸ’¬ jlopp commented on issue "Remove *petertodd.net DNS seed for testnet and mainnet":
(https://github.com/bitcoin/bitcoin/issues/33736#issuecomment-3464196972)
15,000 words from 12+ year old emails?

Further specification is needed to explain how this has anything to do with Peter's control of his DNS seed. All I see are discussions of how different aspects of Bitcoin work.
πŸ€” l0rinc requested changes to a pull request: "refactor: Return uint64_t from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/33724#pullrequestreview-3393067389)
I like the focused commits with good explanations.

I would like to suggest to consider something slightly simpler that may fit the data better.
We're storing these serialized sizes as 64 bits to avoid having to cast when we multiply the value. But we're always serializing these sizes as either variable length ints or 32 bit ones, so it's a bit awkward that a method returns 64 bit value and we just have to remember to cast them to 32 at call sites (otherwise they would serialize incorrectly in a
...
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472821589)
Can we migrate the constant to 64 bits as well?
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/rpc/blockchain.cpp#L1907
```C++
static constexpr uint64_t PER_UTXO_OVERHEAD{sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool)};
```
Or if we will go the other way and mirror storage and only up-cast when needed, it would be:
```C++
static constexpr uint32_t PER_UTXO_OVERHEAD{sizeof(COutPoint) + sizeof(uint32_t) + sizeof(bool)};
```
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472815299)
is there any scenario where where a signed `UniValue` value is different from an unsigned one?
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472880211)
https://github.com/bitcoin/bitcoin/blob/fa128d1488c08d9816f425c73d01db4c1597ee68/src/node/blockstorage.h#L120
should also likely be updated now to:
```C++
static constexpr uint32_t STORAGE_HEADER_BYTES{std::tuple_size_v<MessageStartChars> + sizeof(uint32_t)};
```

----

Note: I understand this works on most 32 bit systems we support, but https://en.cppreference.com/w/cpp/language/types.html indicates it can vary between 16 and 32 bits.
Also, https://learn.microsoft.com/en-us/cpp/cpp/dat
...
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472903256)
Not sure I understand why `GetBlockWeight` is `int64_t`
https://github.com/bitcoin/bitcoin/blob/3c5d1a468199722da620f1f3d8ae3319980a46d5/src/consensus/validation.h#L136
but `MAX_BLOCK_WEIGHT` and `MAX_BLOCK_SERIALIZED_SIZE` are only `unsigned int`
https://github.com/bitcoin/bitcoin/blob/3c5d1a468199722da620f1f3d8ae3319980a46d5/src/consensus/consensus.h#L15
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472976911)
`MAX_FLTR_FILE_SIZE` is only used here
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/index/blockfilterindex.cpp#L39
we should adjust its type to match the usage
```C++
constexpr uint32_t FLTR_FILE_CHUNK_SIZE{0x100000}; // 1 MiB
```
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2472910698)
`FindNextBlockPos` has `unsigned int nAddSize` arg still:
```C++
FlatFilePos BlockManager::FindNextBlockPos(uint32_t nAddSize, unsigned int nHeight, uint64_t nTime)
```
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2473369894)
If we're changing this, we should likely adjust the usage as well:
https://github.com/bitcoin/bitcoin/blob/fab75c2d0332fdb25030abb441363478cd486dc5/src/wallet/spend.cpp#L1085
```C++
int32_t tx_noinputs_size = 0;
```
πŸ’¬ l0rinc commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2473712142)
https://github.com/bitcoin/bitcoin/blob/fa128d1488c08d9816f425c73d01db4c1597ee68/src/flatfile.h#L16
is `int32_t` now so we should update these as well:
```C++
const int32_t nFile = pos.nFile;
if (static_cast<int32_t>(m_blockfile_info.size()) <= nFile) {
```
πŸ’¬ hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3464387417)
Rebased. Some of the feedback has been addressed.

My Guix build:
```
x86_64
366c53942430c0f310fc42445693e7ba1ff5dce25f2df8e42d3c4b044747f385 guix-build-4d31ddc6dfe6/output/dist-archive/bitcoin-4d31ddc6dfe6.tar.gz
1b0c2e70c8f85650a4b4238c74dd22b960b2f4036042d2f8c6b302dbc50ae3b7 guix-build-4d31ddc6dfe6/output/x86_64-w64-mingw32/SHA256SUMS.part
ed292458da2126949ee0a35495ba59272066364d7b458b28576dcd229ed984b5 guix-build-4d31ddc6dfe6/output/x86_64-w64-mingw32/bitcoin-4d31ddc6dfe6-win64-cod
...
πŸ‘‹ hebasto's pull request is ready for review: "guix: Use UCRT runtime for Windows release binaries"
(https://github.com/bitcoin/bitcoin/pull/33593)
πŸ’¬ ajtowns commented on pull request "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)":
(https://github.com/bitcoin/bitcoin/pull/32924#issuecomment-3464418432)
ACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 lgtm

Presumably it would also be possible to use the same signature on a p2wsh tx (`<sig> SWAP CHECKSIG`) which would lose the `CONST_SCRIPTCODE` requirement. Could also presumably just have the script be `CHECKSIG` and provide both the pubkey and the short signature in either p2sh or p2wsh.
πŸ’¬ JeremyRubin commented on issue "Think about tuning the script cache/sigcache ratios":
(https://github.com/bitcoin/bitcoin/issues/10754#issuecomment-3464424185)
@willcl-ark you can also look at the memory model and seeing if/how you can make the cache more parallel during block validation. IIRC there is some unnecessary locking which can be eliminated/reduced to something like one acquire/release per thread and a single thread holding a unique lock to prevent writers.
πŸ’¬ andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475738139)
> But as you said, the thread sanitizer would likely complain about that. We’re accessing the same variable from different threads (main and worker).

It won't complain if we synchronize access to the same non-atomic variable, which we do here because we lock when calling `Submit`.

> The main issue, however, will probably be related to thread visibility; changes made in one thread aren’t guaranteed to be visible to another.

I don't think this is the main issue. If a future change breaks
...
πŸ’¬ umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3464623966)
@l0rinc still fails

1. brew upgrade
2. brew reinstall boost
3. use clang from homebrew (update $PATH)
4. configure logs https://gist.github.com/umrashrf/6ca23317de227f3c8173a0031ced26f8
5. build logs https://gist.github.com/umrashrf/d4e0ef25921f78fe9fc8909bff043ccb
πŸ’¬ l0rinc commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3464678834)
Seems the llvm upgrade was successful, but you still have two boost installations, try removing the old one.
πŸ“ l0rinc opened a pull request: "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled"
(https://github.com/bitcoin/bitcoin/pull/33738)
The new accounting options introduced in https://github.com/bitcoin/bitcoin/pull/32582 can be quite heavy, and not needed when debug logging is disabled.

`PartiallyDownloadedBlock::FillBlock()` and `PeerManagerImpl::SendBlockTransactions()` accumulate transaction sizes for debug logging by calling `GetTotalSize()` in loops, which invokes expensive `GetSerializeSize()` serializations.

Guard the size calculations with `LogAcceptCategory()` checks so serialization only occurs when compact bl
...
⚠️ ukml opened an issue: "Link"
(https://github.com/bitcoin/bitcoin/issues/33739)
This link dos not work