💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946946833)
Isn't it already out of the loop?
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946946833)
Isn't it already out of the loop?
💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946951492)
Oh nvm, there are 2 loops, yes
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946951492)
Oh nvm, there are 2 loops, yes
💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643641231)
@purpleKarrot It's an artifact of a few things:
- The migration from autotools to CMake. autotools made CPPFLAGS a first-class citizen whereas CMake expects users to just pass them in as CXXFLAGS. `APPEND_CPPFLAGS` could probably be rolled into `APPEND_CXXFLAGS`, we just maintain the split because it can be a helpful distinction.
- In many cases `CXXFLAGS` or `CMAKE_CXX_FLAGS` or `CMAKE_CXX_FLAGS_BUILDTYPE` will work, but not always. CMake provides no way to _append_ CXXFLAGS, only to prepend
...
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643641231)
@purpleKarrot It's an artifact of a few things:
- The migration from autotools to CMake. autotools made CPPFLAGS a first-class citizen whereas CMake expects users to just pass them in as CXXFLAGS. `APPEND_CPPFLAGS` could probably be rolled into `APPEND_CXXFLAGS`, we just maintain the split because it can be a helpful distinction.
- In many cases `CXXFLAGS` or `CMAKE_CXX_FLAGS` or `CMAKE_CXX_FLAGS_BUILDTYPE` will work, but not always. CMake provides no way to _append_ CXXFLAGS, only to prepend
...
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946960980)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942419081
> > So if I've been hacking in my local libmultiprocess dir (as I have today), and I do a depends build, it should fail and yell at me that the sources don't match the agreed-upon hash.
>
> Doesn't that apply to any source as well, not only libmultiprocess dir? Should depends build yell if the tree has local mods?
The PR has changed since this comment was written, but the idea is for the depends build to detect if
...
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946960980)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942419081
> > So if I've been hacking in my local libmultiprocess dir (as I have today), and I do a depends build, it should fail and yell at me that the sources don't match the agreed-upon hash.
>
> Doesn't that apply to any source as well, not only libmultiprocess dir? Should depends build yell if the tree has local mods?
The PR has changed since this comment was written, but the idea is for the depends build to detect if
...
📝 Christewart opened a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823)
This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](https://github.com/bitcoin/bitcoin/blob/fb0ada982a73687520c43b8fde480fa5d456f3e1/src/validation.cpp#L4180).
This change was needed in https://github.com/ajtowns/bitcoin/pull/13 w
...
(https://github.com/bitcoin/bitcoin/pull/31823)
This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](https://github.com/bitcoin/bitcoin/blob/fb0ada982a73687520c43b8fde480fa5d456f3e1/src/validation.cpp#L4180).
This change was needed in https://github.com/ajtowns/bitcoin/pull/13 w
...
✅ l0rinc closed an issue: "Proposal: Adopt simple SPDX-License-Identifier Across Bitcoin Codebase"
(https://github.com/bitcoin/bitcoin/issues/29445)
(https://github.com/bitcoin/bitcoin/issues/29445)
💬 l0rinc commented on pull request "fixing-link-Update dependencies.md":
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643688638)
The old link successfully redirects me to the new one, what's the purpose of changing it?
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643688638)
The old link successfully redirects me to the new one, what's the purpose of changing it?
💬 purpleKarrot commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643689227)
> CMake provides no way to append `CXXFLAGS`, ...
Not sure I understand. On the first run, CMake initializes `CMAKE_CXX_FLAGS` by prepending `CMAKE_CXX_FLAGS_INIT` with the environment variable `CXXFLAGS`. See: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_INIT.html
In other words, `CMAKE_CXX_FLAGS_INIT` is appended to `CXXFLAGS`. But you say it provides no way to append to `CXXFLAGS`?
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643689227)
> CMake provides no way to append `CXXFLAGS`, ...
Not sure I understand. On the first run, CMake initializes `CMAKE_CXX_FLAGS` by prepending `CMAKE_CXX_FLAGS_INIT` with the environment variable `CXXFLAGS`. See: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_INIT.html
In other words, `CMAKE_CXX_FLAGS_INIT` is appended to `CXXFLAGS`. But you say it provides no way to append to `CXXFLAGS`?
💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643704920)
I didn't mean CXXFLAGS the env var, I meant CXXFLAGS the concept (as-in, what ultimately gets passed to the compiler). There are just some things that either we set internally or that CMake sets that are impossible to override without affecting something else (by using a None build type for example).`-Ox` is one of them.
So we provide these vars with guaranteed append semantics.
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643704920)
I didn't mean CXXFLAGS the env var, I meant CXXFLAGS the concept (as-in, what ultimately gets passed to the compiler). There are just some things that either we set internally or that CMake sets that are impossible to override without affecting something else (by using a None build type for example).`-Ox` is one of them.
So we provide these vars with guaranteed append semantics.
💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643708491)
Btw, if your argument is "`CXXFLAGS=-DDEBUG_LOCKCONTENTION` would work for the sake of this PR", I agree with you there.
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643708491)
Btw, if your argument is "`CXXFLAGS=-DDEBUG_LOCKCONTENTION` would work for the sake of this PR", I agree with you there.
💬 dedyshkaPexto commented on pull request "fixing-link-Update dependencies.md":
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643712433)
> The old link successfully redirects me to the new one, what's the purpose of changing it?
view of the new link. The old one also still redirects.
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643712433)
> The old link successfully redirects me to the new one, what's the purpose of changing it?
view of the new link. The old one also still redirects.
💬 theuni commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1947001175)
Agreed. Mind addressing this @hebasto?
(https://github.com/bitcoin/bitcoin/pull/31765#discussion_r1947001175)
Agreed. Mind addressing this @hebasto?
💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947016668)
done
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1947016668)
done
💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#issuecomment-2643744332)
> nit: I believe that CheckInvariants() could be outside of the loop in the txdownloadman_impl harness
the checks are now out of the loop, just like core devs
(https://github.com/bitcoin/bitcoin/pull/31810#issuecomment-2643744332)
> nit: I believe that CheckInvariants() could be outside of the loop in the txdownloadman_impl harness
the checks are now out of the loop, just like core devs
💬 theuni commented on pull request "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree":
(https://github.com/bitcoin/bitcoin/pull/31722#discussion_r1947027251)
Could this not be relative to `${PROJECT_SOURCE_DIR}` instead to avoid the two-step copy/process?
(https://github.com/bitcoin/bitcoin/pull/31722#discussion_r1947027251)
Could this not be relative to `${PROJECT_SOURCE_DIR}` instead to avoid the two-step copy/process?
💬 theuni commented on pull request "cmake: Improve compatibility with Python version managers":
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947036941)
I think these should be tested for existence first (in case set by the user), and set to `INTERNAL` ?
(https://github.com/bitcoin/bitcoin/pull/31421#discussion_r1947036941)
I think these should be tested for existence first (in case set by the user), and set to `INTERNAL` ?
💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947042506)
Modern C++ integer division always [truncates toward zero](https://en.cppreference.com/w/c/language/operator_arithmetic#Division). For positive numbers, that’s equivalent to rounding down, but for negatives it rounds up.
Since we need controlled rounding (toward negative infinity when `round_down` is true, or toward positive infinity otherwise), we need to take the sign of the numerator into account to adjust the result when necessary
For example, in [feefrac_tests.cpp](https://github.com/bi
...
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947042506)
Modern C++ integer division always [truncates toward zero](https://en.cppreference.com/w/c/language/operator_arithmetic#Division). For positive numbers, that’s equivalent to rounding down, but for negatives it rounds up.
Since we need controlled rounding (toward negative infinity when `round_down` is true, or toward positive infinity otherwise), we need to take the sign of the numerator into account to adjust the result when necessary
For example, in [feefrac_tests.cpp](https://github.com/bi
...
💬 l0rinc commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947049915)
This is [always used now](https://github.com/bitcoin/bitcoin/commit/0606e66fdbb914f984433d8950f0c32b5fb871f3#diff-eca436ae09a12f8a59ac888c7f4ee6f36e4d12f38c2dab47fee0377ce69fe660R18), right?
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1947049915)
This is [always used now](https://github.com/bitcoin/bitcoin/commit/0606e66fdbb914f984433d8950f0c32b5fb871f3#diff-eca436ae09a12f8a59ac888c7f4ee6f36e4d12f38c2dab47fee0377ce69fe660R18), right?
⚠️ mzumsande opened an issue: "wallet: wrong balance and crash after reorg and unclean shutdown"
(https://github.com/bitcoin/bitcoin/issues/31824)
The wallet can show a wrong balance and/or hit an assert crash in case of a unclean shutdown after a reorg:
Initial state: Wallet and Node are synced to disk (e.g. a Flush just happened), the coinbase of the tip is a wallet tx.
1.) Tip is disconnected due to a reorg. This will change the status of affected wallet txns to Inactive (which will be synced with the wallet db immediately) - but neither the chainstate nor the wallet locator will be flushed.
2.) The node has an unclean shutdown for an
...
(https://github.com/bitcoin/bitcoin/issues/31824)
The wallet can show a wrong balance and/or hit an assert crash in case of a unclean shutdown after a reorg:
Initial state: Wallet and Node are synced to disk (e.g. a Flush just happened), the coinbase of the tip is a wallet tx.
1.) Tip is disconnected due to a reorg. This will change the status of affected wallet txns to Inactive (which will be synced with the wallet db immediately) - but neither the chainstate nor the wallet locator will be flushed.
2.) The node has an unclean shutdown for an
...
🤔 marcofleon reviewed a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602619218)
reACK e107bf78f9d722fcdeb5c1fba5a784dd7747e12f
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602619218)
reACK e107bf78f9d722fcdeb5c1fba5a784dd7747e12f